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

[Caio Marcelo de Oliveira Filho]

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;".


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.

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