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

[Iago Toral Quiroga]

If a source operand in a MOV has source modifiers, then we cannot
copy-propagate it from the parent instruction and remove the MOV.

v2: remove the check for source modifiers from is_move() (Jason)

v3: Put the check for source modifiers back into is_move() since
this function is called from copy_prop_alu_src(). Add source
modifiers checks to is_vec() instead.
---

Jason, I had to revert v2 after noticing this, I did not realize that is_move()
was actually called from another place when you suggested removing the check
from there so I did not think that it could possibly break anything and did not
pass v2 through piglit again. Obviously I was wrong, sorry about that :-(

This version does not produce any regressions in piglit in my IVB laptop.

 src/glsl/nir/nir_opt_copy_propagate.c | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/src/glsl/nir/nir_opt_copy_propagate.c 
b/src/glsl/nir/nir_opt_copy_propagate.c
index 7d8bdd7..cfc8e331 100644
--- a/src/glsl/nir/nir_opt_copy_propagate.c
+++ b/src/glsl/nir/nir_opt_copy_propagate.c
@@ -55,10 +55,15 @@ static bool is_move(nir_alu_instr *instr)
 
 static bool is_vec(nir_alu_instr *instr)
 {
-   for (unsigned i = 0; i < nir_op_infos[instr->op].num_inputs; i++)
+   for (unsigned i = 0; i < nir_op_infos[instr->op].num_inputs; i++) {
   if (!instr->src[i].src.is_ssa)
  return false;
 
+  /* we handle modifiers in a separate pass */
+  if (instr->src[i].abs || instr->src[i].negate)
+ return false;
+   }
+
return instr->op == nir_op_vec2 ||
   instr->op == nir_op_vec3 ||
   instr->op == nir_op_vec4;
-- 
1.9.1

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Jason Ekstrand]

On Fri, Nov 13, 2015 at 12:48 AM, Iago Toral Quiroga  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.
>
> v2: remove the check for source modifiers from is_move() (Jason)
>
> v3: Put the check for source modifiers back into is_move() since
> this function is called from copy_prop_alu_src(). Add source
> modifiers checks to is_vec() instead.
> ---
>
> Jason, I had to revert v2 after noticing this, I did not realize that 
> is_move()
> was actually called from another place when you suggested removing the check
> from there so I did not think that it could possibly break anything and did 
> not
> pass v2 through piglit again. Obviously I was wrong, sorry about that :-(

No worries.  Thanks for catching it.  I thought there was a reason it
was in is_move(), but I couldn't remember what...

> This version does not produce any regressions in piglit in my IVB laptop.

Sounds good.

Reviewed-by: Jason Ekstrand 

>  src/glsl/nir/nir_opt_copy_propagate.c | 7 ++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/src/glsl/nir/nir_opt_copy_propagate.c 
> b/src/glsl/nir/nir_opt_copy_propagate.c
> index 7d8bdd7..cfc8e331 100644
> --- a/src/glsl/nir/nir_opt_copy_propagate.c
> +++ b/src/glsl/nir/nir_opt_copy_propagate.c
> @@ -55,10 +55,15 @@ static bool is_move(nir_alu_instr *instr)
>
>  static bool is_vec(nir_alu_instr *instr)
>  {
> -   for (unsigned i = 0; i < nir_op_infos[instr->op].num_inputs; i++)
> +   for (unsigned i = 0; i < nir_op_infos[instr->op].num_inputs; i++) {
>if (!instr->src[i].src.is_ssa)
>   return false;
>
> +  /* we handle modifiers in a separate pass */
> +  if (instr->src[i].abs || instr->src[i].negate)
> + return false;
> +   }
> +
> return instr->op == nir_op_vec2 ||
>instr->op == nir_op_vec3 ||
>instr->op == nir_op_vec4;
> --
> 1.9.1
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev