[Mesa-dev] [PATCH] i965: Fix move_interpolation_to_top() pass.

[Matt Turner]

On Tue, Jul 26, 2016 at 1:19 PM, Kenneth Graunke <kenneth at whitecape.org> wrote:
> The pass I introduced in commit a2dc11a7818c04d8dc0324e8fcba98d60bae
> was entirely broken.  A missing "break" made the load_interpolated_input
> case always fall through to "default" and hit a "continue", making it
> not actually move any load_interpolated_input intrinsics at all.
> It would only move the simple load_barycentric_* intrinsics, which
> don't emit any code anyway, making it basically useless.
>
> The initial version I sent of the pass worked, but I apparently
> failed to verify that the simplified version in v2 actually worked.
>
> With the obvious fix applied (so we actually tried to move LIIs),

I'm guessing that an "LII" is a "load interpolation intrinsic" or
similar, but I think it'd be better to not abbreviate it since it's
not a common abbreviation.

> I discovered a second bug: we weren't moving the offset SSA def
> to the top, breaking SSA validation.
>
> The new version of the pass actually moves load_interpolated_input
> intrinsics and all their dependencies, as intended.
>
> Papers over GPU hangs on Ivybridge and Baytrail caused by the
> recent NIR FS input rework by restoring the old behavior.
> (I'm not honestly sure why they hang with PLN not at the top.)
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97083
> Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
> ---
>  src/mesa/drivers/dri/i965/brw_fs.cpp | 49 ++++++++++++++++++++----------------
>  1 file changed, 28 insertions(+), 21 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp b/src/mesa/drivers/dri/i965/brw_fs.cpp
> index f9af525..bcd08ac 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
> @@ -6353,38 +6353,45 @@ move_interpolation_to_top(nir_shader *nir)
>           continue;
>
>        nir_block *top = nir_start_block(f->impl);
> +      exec_node *cursor_node = NULL;
>
>        nir_foreach_block(block, f->impl) {
>           if (block == top)
>              continue;
>
> -         nir_foreach_instr_reverse_safe(instr, block) {
> +         nir_foreach_instr_safe(instr, block) {
>              if (instr->type != nir_instr_type_intrinsic)
>                 continue;
>
>              nir_intrinsic_instr *intrin = nir_instr_as_intrinsic(instr);
> -            switch (intrin->intrinsic) {
> -            case nir_intrinsic_load_barycentric_pixel:
> -            case nir_intrinsic_load_barycentric_centroid:
> -            case nir_intrinsic_load_barycentric_sample:
> -               break;
> -            case nir_intrinsic_load_interpolated_input: {
> -               nir_intrinsic_instr *bary_intrinsic =
> -                  nir_instr_as_intrinsic(intrin->src[0].ssa->parent_instr);
> -               nir_intrinsic_op op = bary_intrinsic->intrinsic;
> -
> -               /* Leave interpolateAtSample/Offset() where it is. */
> -               if (op == nir_intrinsic_load_barycentric_at_sample ||
> -                   op == nir_intrinsic_load_barycentric_at_offset)
> -                  continue;
> -            }
> -            default:
> +            if (intrin->intrinsic != nir_intrinsic_load_interpolated_input)
> +               continue;
> +            nir_intrinsic_instr *bary_intrinsic =
> +               nir_instr_as_intrinsic(intrin->src[0].ssa->parent_instr);
> +            nir_intrinsic_op op = bary_intrinsic->intrinsic;
> +
> +            /* Leave interpolateAtSample/Offset() where they are. */
> +            if (op == nir_intrinsic_load_barycentric_at_sample ||
> +                op == nir_intrinsic_load_barycentric_at_offset)
>                 continue;
> -            }
>
> -            exec_node_remove(&instr->node);
> -            exec_list_push_head(&top->instr_list, &instr->node);
> -            instr->block = top;
> +            nir_instr *move[3] = {
> +               &bary_intrinsic->instr,
> +               intrin->src[1].ssa->parent_instr,
> +               instr
> +            };
> +
> +            for (int i = 0; i < 3; i++) {

s/3/ARRAY_SIZE(move)/ ?

> +               if (move[i]->block != top) {
> +                  move[i]->block = top;
> +                  exec_node_remove(&move[i]->node);
> +                  if (cursor_node)
> +                     exec_node_insert_after(cursor_node, &move[i]->node);
> +                  else
> +                     exec_list_push_head(&top->instr_list, &move[i]->node);

I'd personally use braces here.

> +                  cursor_node = &move[i]->node;
> +               }
> +            }
>           }
>        }
>        nir_metadata_preserve(f->impl, (nir_metadata)
> --
> 2.9.0

The logic all looks right.

Reviewed-by: Matt Turner <mattst88 at gmail.com>