Patchwork combine BIT_FIELD_REF and VEC_PERM_EXPR

login
register
mail settings
Submitter Marc Glisse
Date Sept. 8, 2012, 8:17 a.m.
Message ID <alpine.DEB.2.02.1209080930480.5700@stedding.saclay.inria.fr>
Download mbox | patch
Permalink /patch/182506/
State New
Headers show

Comments

Marc Glisse - Sept. 8, 2012, 8:17 a.m.
On Mon, 3 Sep 2012, Richard Guenther wrote:

> Please do the early outs where you compute the arguments.  Thus, right
> after getting op0 in this case or right after computing n for the n != 1 check.
>
> I think you need to verify that the type of 'op' is actually the element type
> of op0.  The BIT_FIELD_REF can happily access elements two and three
> of { 1, 2, 3, 4 } as a long for example.  See the BIT_FIELD_REF foldings
> in fold-const.c.

On Mon, 3 Sep 2012, Richard Guenther wrote:

> If you use fold_build3 you need to check that the result is in expected form
> (a is_gimple_invariant or an SSA_NAME).

I first tried this:

+      if (TREE_CODE (tem) != SSA_NAME
+         && TREE_CODE (tem) != BIT_FIELD_REF
+         && !is_gimple_min_invariant (tem))
+       return false;

but then I thought that fold_stmt probably does the right thing (?) so I
switched to it.

>> Now that I look at this line, I wonder if I am missing some unshare_expr for
>> p and/or op1.
>
> If either is a CONSTRUCTOR and its def stmt is not removed and it survives
> into tem then yes ...

I added an unconditional unshare_expr. I guess it would be possible to 
look at if the permutation is only used once and in that case maybe not 
call unshare_expr (?) and call remove_prop_source_from_use at the end, but 
it gets a bit complicated and I don't think it helps compared to waiting 
for the next DCE pass.

>>> Please also add handling of code == CONSTRUCTOR.
>>
>> The cases I tried were already handled by fre1. I can add code for
>> constructor, but I'll need to look for a testcase first. Can that go to a
>> different patch?
>
> Yes.

This is currently handled by FRE. The only testcase I have that reaches 
forwprop is bit_field_ref (vec_perm_expr (constructor)) and will require 
me to disable this patch so I can test that one...


New version of the patch (I really think I already regtested it, but I'll 
do it again to be sure):

2012-09-08  Marc Glisse  <marc.glisse@inria.fr>

gcc/
 	* tree-ssa-forwprop.c (simplify_bitfield): New function.
 	(ssa_forward_propagate_and_combine): Call it.

gcc/testsuite/
 	* gcc.dg/tree-ssa/forwprop-21.c: New testcase.
Richard Guenther - Sept. 10, 2012, 12:04 p.m.
On Sat, Sep 8, 2012 at 10:17 AM, Marc Glisse <marc.glisse@inria.fr> wrote:
> On Mon, 3 Sep 2012, Richard Guenther wrote:
>
>> Please do the early outs where you compute the arguments.  Thus, right
>> after getting op0 in this case or right after computing n for the n != 1
>> check.
>>
>> I think you need to verify that the type of 'op' is actually the element
>> type
>> of op0.  The BIT_FIELD_REF can happily access elements two and three
>> of { 1, 2, 3, 4 } as a long for example.  See the BIT_FIELD_REF foldings
>> in fold-const.c.
>
>
> On Mon, 3 Sep 2012, Richard Guenther wrote:
>
>> If you use fold_build3 you need to check that the result is in expected
>> form
>> (a is_gimple_invariant or an SSA_NAME).
>
>
> I first tried this:
>
> +      if (TREE_CODE (tem) != SSA_NAME
> +         && TREE_CODE (tem) != BIT_FIELD_REF
> +         && !is_gimple_min_invariant (tem))
> +       return false;
>
> but then I thought that fold_stmt probably does the right thing (?) so I
> switched to it.

I should indeed.

>
>>> Now that I look at this line, I wonder if I am missing some unshare_expr
>>> for
>>> p and/or op1.
>>
>>
>> If either is a CONSTRUCTOR and its def stmt is not removed and it survives
>> into tem then yes ...
>
>
> I added an unconditional unshare_expr. I guess it would be possible to look
> at if the permutation is only used once and in that case maybe not call
> unshare_expr (?) and call remove_prop_source_from_use at the end, but it
> gets a bit complicated and I don't think it helps compared to waiting for
> the next DCE pass.
>
>
>>>> Please also add handling of code == CONSTRUCTOR.
>>>
>>>
>>> The cases I tried were already handled by fre1. I can add code for
>>> constructor, but I'll need to look for a testcase first. Can that go to a
>>> different patch?
>>
>>
>> Yes.
>
>
> This is currently handled by FRE. The only testcase I have that reaches
> forwprop is bit_field_ref (vec_perm_expr (constructor)) and will require me
> to disable this patch so I can test that one...
>
>
> New version of the patch (I really think I already regtested it, but I'll do
> it again to be sure):
>
> 2012-09-08  Marc Glisse  <marc.glisse@inria.fr>
>
>
> gcc/
>         * tree-ssa-forwprop.c (simplify_bitfield): New function.
>         (ssa_forward_propagate_and_combine): Call it.
>
> gcc/testsuite/
>         * gcc.dg/tree-ssa/forwprop-21.c: New testcase.
>
> --
> Marc Glisse
> Index: tree-ssa-forwprop.c
> ===================================================================
> --- tree-ssa-forwprop.c (revision 191089)
> +++ tree-ssa-forwprop.c (working copy)
> @@ -2567,20 +2567,92 @@ combine_conversions (gimple_stmt_iterato
>               gimple_assign_set_rhs_code (stmt, CONVERT_EXPR);
>               update_stmt (stmt);
>               return remove_prop_source_from_use (op0) ? 2 : 1;
>             }
>         }
>      }
>
>    return 0;
>  }
>
> +/* Combine an element access with a shuffle.  Returns true if there were
> +   any changes made, else it returns false.  */
> +
> +static bool
> +simplify_bitfield (gimple_stmt_iterator *gsi)

simplify_bitfield_ref

Ok with that change.

Thanks,
Richard.

> +{
> +  gimple stmt = gsi_stmt (*gsi);
> +  gimple def_stmt;
> +  tree op, op0, op1, op2;
> +  tree elem_type;
> +  unsigned idx, n, size;
> +  enum tree_code code;
> +
> +  op = gimple_assign_rhs1 (stmt);
> +  gcc_checking_assert (TREE_CODE (op) == BIT_FIELD_REF);
> +
> +  op0 = TREE_OPERAND (op, 0);
> +  if (TREE_CODE (op0) != SSA_NAME
> +      || TREE_CODE (TREE_TYPE (op0)) != VECTOR_TYPE)
> +    return false;
> +
> +  elem_type = TREE_TYPE (TREE_TYPE (op0));
> +  if (TREE_TYPE (op) != elem_type)
> +    return false;
> +
> +  size = TREE_INT_CST_LOW (TYPE_SIZE (elem_type));
> +  op1 = TREE_OPERAND (op, 1);
> +  n = TREE_INT_CST_LOW (op1) / size;
> +  if (n != 1)
> +    return false;
> +
> +  def_stmt = SSA_NAME_DEF_STMT (op0);
> +  if (!def_stmt || !is_gimple_assign (def_stmt)
> +      || !can_propagate_from (def_stmt))
> +    return false;
> +
> +  op2 = TREE_OPERAND (op, 2);
> +  idx = TREE_INT_CST_LOW (op2) / size;
> +
> +  code = gimple_assign_rhs_code (def_stmt);
> +
> +  if (code == VEC_PERM_EXPR)
> +    {
> +      tree p, m, index, tem;
> +      unsigned nelts;
> +      m = gimple_assign_rhs3 (def_stmt);
> +      if (TREE_CODE (m) != VECTOR_CST)
> +       return false;
> +      nelts = VECTOR_CST_NELTS (m);
> +      idx = TREE_INT_CST_LOW (VECTOR_CST_ELT (m, idx));
> +      idx %= 2 * nelts;
> +      if (idx < nelts)
> +       {
> +         p = gimple_assign_rhs1 (def_stmt);
> +       }
> +      else
> +       {
> +         p = gimple_assign_rhs2 (def_stmt);
> +         idx -= nelts;
> +       }
> +      index = build_int_cst (TREE_TYPE (TREE_TYPE (m)), idx * size);
> +      tem = build3 (BIT_FIELD_REF, TREE_TYPE (op),
> +                        unshare_expr (p), op1, index);
> +      gimple_assign_set_rhs1 (stmt, tem);
> +      fold_stmt (gsi);
> +      update_stmt (gsi_stmt (*gsi));
> +      return true;
> +    }
> +
> +  return false;
> +}
> +
>  /* Determine whether applying the 2 permutations (mask1 then mask2)
>     gives back one of the input.  */
>
>  static int
>  is_combined_permutation_identity (tree mask1, tree mask2)
>  {
>    tree mask;
>    unsigned int nelts, i, j;
>    bool maybe_identity1 = true;
>    bool maybe_identity2 = true;
> @@ -2825,20 +2897,22 @@ ssa_forward_propagate_and_combine (void)
>                       cfg_changed = true;
>                     changed = did_something != 0;
>                   }
>                 else if (code == VEC_PERM_EXPR)
>                   {
>                     int did_something = simplify_permutation (&gsi);
>                     if (did_something == 2)
>                       cfg_changed = true;
>                     changed = did_something != 0;
>                   }
> +               else if (code == BIT_FIELD_REF)
> +                 changed = simplify_bitfield (&gsi);
>                 break;
>               }
>
>             case GIMPLE_SWITCH:
>               changed = simplify_gimple_switch (stmt);
>               break;
>
>             case GIMPLE_COND:
>               {
>                 int did_something;
> Index: testsuite/gcc.dg/tree-ssa/forwprop-21.c
> ===================================================================
> --- testsuite/gcc.dg/tree-ssa/forwprop-21.c     (revision 0)
> +++ testsuite/gcc.dg/tree-ssa/forwprop-21.c     (revision 0)
> @@ -0,0 +1,13 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O -fdump-tree-optimized" } */
> +typedef int v4si __attribute__ ((vector_size (4 * sizeof(int))));
> +
> +int
> +test (v4si *x, v4si *y)
> +{
> +  v4si m = { 2, 3, 6, 5 };
> +  v4si z = __builtin_shuffle (*x, *y, m);
> +  return z[2];
> +}
> +/* { dg-final { scan-tree-dump-not "VEC_PERM_EXPR" "optimized" } } */
> +/* { dg-final { cleanup-tree-dump "optimized" } } */
>
> Property changes on: testsuite/gcc.dg/tree-ssa/forwprop-21.c
> ___________________________________________________________________
> Added: svn:eol-style
>    + native
> Added: svn:keywords
>    + Author Date Id Revision URL
>
>

Patch

Index: tree-ssa-forwprop.c

===================================================================
--- tree-ssa-forwprop.c	(revision 191089)

+++ tree-ssa-forwprop.c	(working copy)

@@ -2567,20 +2567,92 @@  combine_conversions (gimple_stmt_iterato

 	      gimple_assign_set_rhs_code (stmt, CONVERT_EXPR);
 	      update_stmt (stmt);
 	      return remove_prop_source_from_use (op0) ? 2 : 1;
 	    }
 	}
     }
 
   return 0;
 }
 
+/* Combine an element access with a shuffle.  Returns true if there were

+   any changes made, else it returns false.  */

+ 

+static bool

+simplify_bitfield (gimple_stmt_iterator *gsi)

+{

+  gimple stmt = gsi_stmt (*gsi);

+  gimple def_stmt;

+  tree op, op0, op1, op2;

+  tree elem_type;

+  unsigned idx, n, size;

+  enum tree_code code;

+

+  op = gimple_assign_rhs1 (stmt);

+  gcc_checking_assert (TREE_CODE (op) == BIT_FIELD_REF);

+

+  op0 = TREE_OPERAND (op, 0);

+  if (TREE_CODE (op0) != SSA_NAME

+      || TREE_CODE (TREE_TYPE (op0)) != VECTOR_TYPE)

+    return false;

+

+  elem_type = TREE_TYPE (TREE_TYPE (op0));

+  if (TREE_TYPE (op) != elem_type)

+    return false;

+

+  size = TREE_INT_CST_LOW (TYPE_SIZE (elem_type));

+  op1 = TREE_OPERAND (op, 1);

+  n = TREE_INT_CST_LOW (op1) / size;

+  if (n != 1)

+    return false;

+

+  def_stmt = SSA_NAME_DEF_STMT (op0);

+  if (!def_stmt || !is_gimple_assign (def_stmt)

+      || !can_propagate_from (def_stmt))

+    return false;

+

+  op2 = TREE_OPERAND (op, 2);

+  idx = TREE_INT_CST_LOW (op2) / size;

+

+  code = gimple_assign_rhs_code (def_stmt);

+

+  if (code == VEC_PERM_EXPR)

+    {

+      tree p, m, index, tem;

+      unsigned nelts;

+      m = gimple_assign_rhs3 (def_stmt);

+      if (TREE_CODE (m) != VECTOR_CST)

+	return false;

+      nelts = VECTOR_CST_NELTS (m);

+      idx = TREE_INT_CST_LOW (VECTOR_CST_ELT (m, idx));

+      idx %= 2 * nelts;

+      if (idx < nelts)

+	{

+	  p = gimple_assign_rhs1 (def_stmt);

+	}

+      else

+	{

+	  p = gimple_assign_rhs2 (def_stmt);

+	  idx -= nelts;

+	}

+      index = build_int_cst (TREE_TYPE (TREE_TYPE (m)), idx * size);

+      tem = build3 (BIT_FIELD_REF, TREE_TYPE (op),

+			 unshare_expr (p), op1, index);

+      gimple_assign_set_rhs1 (stmt, tem);

+      fold_stmt (gsi);

+      update_stmt (gsi_stmt (*gsi));

+      return true;

+    }

+

+  return false;

+}

+

 /* Determine whether applying the 2 permutations (mask1 then mask2)
    gives back one of the input.  */
 
 static int
 is_combined_permutation_identity (tree mask1, tree mask2)
 {
   tree mask;
   unsigned int nelts, i, j;
   bool maybe_identity1 = true;
   bool maybe_identity2 = true;
@@ -2825,20 +2897,22 @@  ssa_forward_propagate_and_combine (void)

 		      cfg_changed = true;
 		    changed = did_something != 0;
 		  }
 		else if (code == VEC_PERM_EXPR)
 		  {
 		    int did_something = simplify_permutation (&gsi);
 		    if (did_something == 2)
 		      cfg_changed = true;
 		    changed = did_something != 0;
 		  }
+		else if (code == BIT_FIELD_REF)

+		  changed = simplify_bitfield (&gsi);

 		break;
 	      }
 
 	    case GIMPLE_SWITCH:
 	      changed = simplify_gimple_switch (stmt);
 	      break;
 
 	    case GIMPLE_COND:
 	      {
 		int did_something;
Index: testsuite/gcc.dg/tree-ssa/forwprop-21.c

===================================================================
--- testsuite/gcc.dg/tree-ssa/forwprop-21.c	(revision 0)

+++ testsuite/gcc.dg/tree-ssa/forwprop-21.c	(revision 0)

@@ -0,0 +1,13 @@ 

+/* { dg-do compile } */

+/* { dg-options "-O -fdump-tree-optimized" } */

+typedef int v4si __attribute__ ((vector_size (4 * sizeof(int))));

+

+int

+test (v4si *x, v4si *y)

+{

+  v4si m = { 2, 3, 6, 5 };

+  v4si z = __builtin_shuffle (*x, *y, m);

+  return z[2];

+}

+/* { dg-final { scan-tree-dump-not "VEC_PERM_EXPR" "optimized" } } */

+/* { dg-final { cleanup-tree-dump "optimized" } } */