Re: [Mesa-dev] [PATCH v4 029/129] nir: Support deref instructions in lower_wpos_ytransform

[Danylo Piliaiev]

Hi,

I was running cppcheck on Mesa and found the issue mentioned here:

This can't possibly work.  The function immediately dereferences
fragcoord, so it can't legally be NULL.  Which then begs the question,
how on earth did it work before?  It would just read a non-existent
variable out of the intrinsic and use that (i.e. NULL anyway).

So, I did a bit of poking around, and noticed that everybody calls this
before nir_lower_system_values, so I think this case could simply be
deleted.  Not sure whether to do that before or after your patch.

We should really delete it at some point.

i965 calls nir_lower_system_values before calling nir_lower_wpos_ytransform,
but lower_fragcoord(state, intr, NULL); seems still to be unreachable.
The question is, is it still ok to remove it?

- Danil


On 12.06.18 22:50, Kenneth Graunke wrote:

On Thursday, May 31, 2018 10:02:12 PM PDT Jason Ekstrand wrote:

Reviewed-by: Caio Marcelo de Oliveira Filho<caio.olive...@intel.com>
---
  src/compiler/nir/nir_lower_wpos_ytransform.c | 53 ++++++++++++++++++++++------
  1 file changed, 42 insertions(+), 11 deletions(-)

diff --git a/src/compiler/nir/nir_lower_wpos_ytransform.c 
b/src/compiler/nir/nir_lower_wpos_ytransform.c
index 9cb5c71..6212702 100644
--- a/src/compiler/nir/nir_lower_wpos_ytransform.c
+++ b/src/compiler/nir/nir_lower_wpos_ytransform.c
@@ -77,11 +77,10 @@ nir_cmp(nir_builder *b, nir_ssa_def *src0, nir_ssa_def 
*src1, nir_ssa_def *src2)
  /* see emit_wpos_adjustment() in st_mesa_to_tgsi.c */
  static void
  emit_wpos_adjustment(lower_wpos_ytransform_state *state,
-                     nir_intrinsic_instr *intr,
+                     nir_intrinsic_instr *intr, nir_variable *fragcoord,
                       bool invert, float adjX, float adjY[2])
  {
     nir_builder *b = &state->b;
-   nir_variable *fragcoord = intr->variables[0]->var;
     nir_ssa_def *wpostrans, *wpos_temp, *wpos_temp_y, *wpos_input;

assert(intr->dest.is_ssa);

@@ -144,10 +143,10 @@ emit_wpos_adjustment(lower_wpos_ytransform_state *state,
  }

static void

-lower_fragcoord(lower_wpos_ytransform_state *state, nir_intrinsic_instr *intr)
+lower_fragcoord(lower_wpos_ytransform_state *state,
+                nir_intrinsic_instr *intr, nir_variable *fragcoord)
  {
     const nir_lower_wpos_ytransform_options *options = state->options;
-   nir_variable *fragcoord = intr->variables[0]->var;
     float adjX = 0.0f;
     float adjY[2] = { 0.0f, 0.0f };
     bool invert = false;
@@ -229,7 +228,7 @@ lower_fragcoord(lower_wpos_ytransform_state *state, 
nir_intrinsic_instr *intr)
        }
     }

- emit_wpos_adjustment(state, intr, invert, adjX, adjY);

+   emit_wpos_adjustment(state, intr, fragcoord, invert, adjX, adjY);
  }

/* turns 'fddy(p)' into 'fddy(fmul(p, transform.x))' */

@@ -253,7 +252,25 @@ lower_fddy(lower_wpos_ytransform_state *state, 
nir_alu_instr *fddy)
        fddy->src[0].swizzle[i] = MIN2(i, pt->num_components - 1);
  }

-/* Multiply interp_var_at_offset's offset by transform.x to flip it. */

+/* Multiply interp_deref_at_offset's offset by transform.x to flip it. */
+static void
+lower_interp_deref_at_offset(lower_wpos_ytransform_state *state,
+                           nir_intrinsic_instr *interp)
+{
+   nir_builder *b = &state->b;
+   nir_ssa_def *offset;
+   nir_ssa_def *flip_y;
+
+   b->cursor = nir_before_instr(&interp->instr);
+
+   offset = nir_ssa_for_src(b, interp->src[1], 2);
+   flip_y = nir_fmul(b, nir_channel(b, offset, 1),
+                        nir_channel(b, get_transform(state), 0));
+   nir_instr_rewrite_src(&interp->instr, &interp->src[1],
+                         nir_src_for_ssa(nir_vec2(b, nir_channel(b, offset, 0),
+                                                     flip_y)));
+}
+
  static void
  lower_interp_var_at_offset(lower_wpos_ytransform_state *state,
                             nir_intrinsic_instr *interp)
@@ -298,7 +315,21 @@ lower_wpos_ytransform_block(lower_wpos_ytransform_state 
*state, nir_block *block
     nir_foreach_instr_safe(instr, block) {
        if (instr->type == nir_instr_type_intrinsic) {
           nir_intrinsic_instr *intr = nir_instr_as_intrinsic(instr);
-         if (intr->intrinsic == nir_intrinsic_load_var) {
+         if (intr->intrinsic == nir_intrinsic_load_deref) {
+            nir_deref_instr *deref = nir_src_as_deref(intr->src[0]);
+            nir_variable *var = nir_deref_instr_get_variable(deref);
+
+            if ((var->data.mode == nir_var_shader_in &&
+                 var->data.location == VARYING_SLOT_POS) ||
+                (var->data.mode == nir_var_system_value &&
+                 var->data.location == SYSTEM_VALUE_FRAG_COORD)) {
+               /* gl_FragCoord should not have array/struct derefs: */
+               lower_fragcoord(state, intr, var);
+            } else if (var->data.mode == nir_var_system_value &&
+                       var->data.location == SYSTEM_VALUE_SAMPLE_POS) {
+               lower_load_sample_pos(state, intr);
+            }

Lots of duplication here :(  But I suppose the other case is going away
at the end of the series, so no real point in tidying...

+         } else if (intr->intrinsic == nir_intrinsic_load_var) {
              nir_deref_var *dvar = intr->variables[0];
              nir_variable *var = dvar->var;

@@ -308,16 +339,18 @@ lower_wpos_ytransform_block(lower_wpos_ytransform_state *state, nir_block *block

                   var->data.location == SYSTEM_VALUE_FRAG_COORD)) {
                 /* gl_FragCoord should not have array/struct derefs: */
                 assert(dvar->deref.child == NULL);
-               lower_fragcoord(state, intr);
+               lower_fragcoord(state, intr, var);
              } else if (var->data.mode == nir_var_system_value &&
                         var->data.location == SYSTEM_VALUE_SAMPLE_POS) {
                 assert(dvar->deref.child == NULL);
                 lower_load_sample_pos(state, intr);
              }
           } else if (intr->intrinsic == nir_intrinsic_load_frag_coord) {
-            lower_fragcoord(state, intr);
+            lower_fragcoord(state, intr, NULL);

This can't possibly work.  The function immediately dereferences
fragcoord, so it can't legally be NULL.  Which then begs the question,
how on earth did it work before?  It would just read a non-existent
variable out of the intrinsic and use that (i.e. NULL anyway).

So, I did a bit of poking around, and noticed that everybody calls this
before nir_lower_system_values, so I think this case could simply be
deleted.  Not sure whether to do that before or after your patch.

It looks fine other than that pre-existing bit of broken code, so
Reviewed-by: Kenneth Graunke<kenn...@whitecape.org>

We should really delete it at some point.

           } else if (intr->intrinsic == nir_intrinsic_load_sample_pos) {
              lower_load_sample_pos(state, intr);
+         } else if (intr->intrinsic == nir_intrinsic_interp_deref_at_offset) {
+            lower_interp_deref_at_offset(state, intr);
           } else if (intr->intrinsic == nir_intrinsic_interp_var_at_offset) {
              lower_interp_var_at_offset(state, intr);
           }
@@ -352,8 +385,6 @@ nir_lower_wpos_ytransform(nir_shader *shader,
        .shader = shader,
     };

- nir_assert_lowered_derefs(shader, nir_lower_load_store_derefs);

-
     assert(shader->info.stage == MESA_SHADER_FRAGMENT);

nir_foreach_function(function, shader) {

_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev