<html> <head> <meta http-equiv="Content-Type" content="text/html; charset=windows-1252"> </head> <body text="#000000" bgcolor="#FFFFFF"> Hi, I was running cppcheck on Mesa and found the issue mentioned here: <blockquote type="cite"> <pre wrap="">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.</pre> </blockquote> i965 calls <tt>nir_lower_system_values</tt> before calling <tt>nir_lower_wpos_ytransform</tt>, but <tt>lower_fragcoord(state, intr, NULL);</tt> seems still to be unreachable. The question is, is it still ok to remove it? - Danil <div class="moz-cite-prefix">On 12.06.18 22:50, Kenneth Graunke wrote: </div> <blockquote type="cite" cite="mid:1752896.CDNbtZnBIc@kirito"> <pre wrap="">On Thursday, May 31, 2018 10:02:12 PM PDT Jason Ekstrand wrote: </pre> <blockquote type="cite"> <pre wrap="">Reviewed-by: Caio Marcelo de Oliveira Filho <a class="moz-txt-link-rfc2396E" href="mailto:caio.oliveira@intel.com"><caio.oliveira@intel.com></a> --- 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); + } </pre> </blockquote> <pre wrap="">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... </pre> <blockquote type="cite"> <pre wrap="">+ } 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); </pre> </blockquote> <pre wrap="">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 <a class="moz-txt-link-rfc2396E" href="mailto:kenneth@whitecape.org"><kenneth@whitecape.org></a> We should really delete it at some point. </pre> <blockquote type="cite"> <pre wrap=""> } 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) { </pre> </blockquote> <fieldset class="mimeAttachmentHeader"></fieldset> <pre wrap="">_______________________________________________ mesa-dev mailing list <a class="moz-txt-link-abbreviated" href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> <a class="moz-txt-link-freetext" href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> </pre> </blockquote> </body> </html>