[Mesa-dev] [PATCH] nir/copy_propagate: do not copy-propagate MOV srcs with source modifiers

[Jason Ekstrand]

On Wed, Nov 11, 2015 at 12:20 AM, Iago Toral Quiroga <itoral at igalia.com> wrote:
> If a source operand in a MOV has source modifiers, then we cannot
> copy-propagate it from the parent instruction and remove the MOV.
> ---
>
> I noticed this while debugging some regressions introduced with the fp64
> code. Basically, I had code similar to this:
>
> vec4 ssa1 = intrincisc1 (...) (...)
> vec2 ssa2 = vec2 ssa1 -ssa1.y
> vec4 ssa3 = intrinsic2 (ssa2) (...)
>
> that would be turned into this by copy propagation:
>
> vec4 ssa1 = some intrincisc
> vec4 ssa2 = some intrinsic (ssa1)
>
> which is obviously not correct. This was happening because
> is_swizzleless_move checked that the MOV/vecN operation that generates
> the value we want to copy-propagate does not incorporate swizzling, but
> it ignored the case where it also added source modifiers, in which case
> we can't copy-propagate either.
>
> Of course, now that we have made vecN operations unsigned again, that example
> can't happen because lower_to_source_mods won't produce things like that, but
> I figured that the patch would still make sense, since it fixes a case where
> copy-propagation won't work as intended, even if we are not currently triggering
> it (at least not with vecN operations).
>
>  src/glsl/nir/nir_opt_copy_propagate.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/src/glsl/nir/nir_opt_copy_propagate.c b/src/glsl/nir/nir_opt_copy_propagate.c
> index 7d8bdd7..7caa4b7 100644
> --- a/src/glsl/nir/nir_opt_copy_propagate.c
> +++ b/src/glsl/nir/nir_opt_copy_propagate.c
> @@ -65,9 +65,12 @@ static bool is_vec(nir_alu_instr *instr)
>  }
>
>  static bool
> -is_swizzleless_move(nir_alu_instr *instr)
> +is_simple_move(nir_alu_instr *instr)
>  {
>     if (is_move(instr)) {
> +      if (instr->src[0].negate || instr->src[0].abs)
> +         return false;

We already do this in is_move() but it might be best to move it here...

> +
>        for (unsigned i = 0; i < 4; i++) {
>           if (!((instr->dest.write_mask >> i) & 1))
>              break;
> @@ -81,6 +84,9 @@ is_swizzleless_move(nir_alu_instr *instr)
>           if (instr->src[i].swizzle[0] != i)
>              return false;
>
> +         if (instr->src[i].negate || instr->src[i].abs)
> +            return false;

We should either move the one in is_move or we should put this into
is_vec.  I think I'd be more of a fan of the former.
--Jason

> +
>           if (def == NULL) {
>              def = instr->src[i].src.ssa;
>           } else if (instr->src[i].src.ssa != def) {
> @@ -107,7 +113,7 @@ copy_prop_src(nir_src *src, nir_instr *parent_instr, nir_if *parent_if)
>        return false;
>
>     nir_alu_instr *alu_instr = nir_instr_as_alu(src_instr);
> -   if (!is_swizzleless_move(alu_instr))
> +   if (!is_simple_move(alu_instr))
>        return false;
>
>     /* Don't let copy propagation land us with a phi that has more
> --
> 1.9.1
>