[Mesa-dev] [PATCH] nir: propagate known constant values into the if-then branch

[Timothy Arceri]

On 23/1/19 11:27 am, Caio Marcelo de Oliveira Filho wrote:
> Hi,
> 
>> Did you look at any of the HURT? The problem I was seeing was this could end
>> up stopping copy propagation from working on some UBOs etc.
> 
> They were not UBO cases like yours, but looking at them I've found a
> different problem.
> 
> 
>> However with this patch we end up with:
>>
>> load UBO at offset offset_a;
>>
>> if (offset_a == 10) {
>>     load UBO at offset 10;
>>     ...
>> }
>>
>> copy prop no longer recognizes that we are reusing the same UBO and reloads
>> it again.
> 
> Note that: when/if we keep derefs for UBOs longer enough to let
> nir_opt_copy_prop_vars act on them, this won't be a problem as we
> perform those propagations.  I'm not sure you are doing the same,
> though.
> 
> See also
> https://gitlab.freedesktop.org/jekstrand/mesa/commits/wip/nir-ssbo-opt
> -- in particular the last few patches.
> 
> 
>> +static bool
>> +opt_for_known_values(nir_builder *b, nir_if *nif)
>> +{
>> +   bool progress = false;
> 
> Turns out we need this one...
> 
> 
>> +   assert(nif->condition.is_ssa);
>> +   nir_ssa_def *if_cond = nif->condition.ssa;
>> +
>> +   if (if_cond->parent_instr->type != nir_instr_type_alu)
>> +      return false;
>> +
>> +   nir_alu_instr *alu = nir_instr_as_alu(if_cond->parent_instr);
>> +   switch (alu->op) {
>> +   case nir_op_feq:
>> +   case nir_op_ieq: {
>> +      nir_load_const_instr *load_const = NULL;
>> +      nir_ssa_def *unknown_val = NULL;
>> +
>> +      nir_ssa_def *src0 = alu->src[0].src.ssa;
>> +      nir_ssa_def *src1 = alu->src[1].src.ssa;
>> +      if (src0a->parent_instr->type == nir_instr_type_load_const) {
>> +         load_const = nir_instr_as_load_const(src0->parent_instr);
>> +         unknown_val = src1;
>> +      } else if (src1->parent_instr->type == nir_instr_type_load_const) {
>> +         load_const = nir_instr_as_load_const(src1->parent_instr);
>> +         unknown_val = src0;
>> +      }
>> +
>> +      if (!load_const)
>> +        return false;
>> +
>> +      /* TODO: remove this and support swizzles? */
>> +      if (unknown_val->num_components != 1)
>> +        return false;
>> +
>> +      /* Replace unknown ssa uses with the known constant */
>> +      nir_foreach_use_safe(use_src, unknown_val) {
>> +         nir_cursor cursor = nir_before_src(use_src, false);
>> +         nir_block *use_block = nir_cursor_current_block(cursor);
>> +         if (nir_block_dominates(nir_if_first_then_block(nif), use_block)) {
>> +            nir_instr_rewrite_src(use_src->parent_instr, use_src,
>> +                                  nir_src_for_ssa(&load_const->def));
>> +            return true;
> 
> ...but we are returning too early here.  This should be "progress =
> true;" instead of the return, otherwise we just replace a single use.
> Then fix the final return to "return progress;".

Ah yes. Good catch.

> 
> 
> The different problem I've found was that uses in the phi instruction
> after the then/else blocks was being replaced, causing churn in the
> optimizations further on.  As a hack, I've ignored phi instructions in
> the use loop above.  HURTs are gone and HELPs continued.
> 
> The real issue seems to be use_block not being what we expect when we
> have a use_src from a phi instruction.  For example, in the snippet
> below
> 
>      vec1 1 ssa_2543 = ieq ssa_7429, ssa_6
>      /* succs: block_9 block_10 */
>      if ssa_2543 {
>              block block_9:
>              /* preds: block_8 */
>              vec2 32 ssa_2550 = vec2 ssa_7744, ssa_7745
>              vec4 32 ssa_2553 = txl ssa_468 (texture_deref), ssa_468 (sampler_deref), ssa_2550 (coord), ssa_6 (lod),
>              vec1 32 ssa_7434 = imov ssa_2553.w
>              /* succs: block_11 */
>      } else {
>              block block_10:
>              /* preds: block_8 */
>              /* succs: block_11 */
>      }
>      block block_11:
>      /* preds: block_9 block_10 */
>      vec1 32 ssa_7433 = phi block_9: ssa_7420, block_10: ssa_7420
>      vec1 32 ssa_7436 = phi block_9: ssa_7434, block_10: ssa_7423
>      vec1 32 ssa_7439 = phi block_9: ssa_7426, block_10: ssa_7426
>      vec1 32 ssa_7442 = phi block_9: ssa_7429, block_10: ssa_7429
> 
> the use_src that is part of the instruction "... ssa_7442 = phi ...",
> was being considered use_block == 9 (the 'then'-block), so replace was
> happening.

I'm not sure I see the problem, replacing the phi src is valid. The 
question is why is it causing HURT?

> 
> Unless I'm missing something, replacing nir_before_src /
> nir_cursor_current_block with use_src->parent_instr->block is what we
> want here.
> 
> 
> 	Caio
>