[Mesa-dev] [PATCH v4 029/129] nir: Support deref instructions in lower_wpos_ytransform
Danylo Piliaiev
danylo.piliaiev at gmail.com
Wed Jul 18 09:15:34 UTC 2018
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.oliveira at 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<kenneth at 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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180718/265a33a4/attachment-0001.html>
More information about the mesa-dev
mailing list