[PATCH] Fix PR 52631 (VN does not use simplified expression for lookup)

[PATCH] Fix PR 52631 (VN does not use simplified expression for lookup)

[Andrew Pinski]

[-- Attachment #1: Type: text/plain, Size: 541 bytes --]

Hi,
  Before tuples was introduced, VN used to lookup the simplified
expression to see if it was available already and use that instead of
the non simplified one.  This patch adds the support back to VN to do
exactly that.

OK? Bootstrapped and tested on x86_64-linux-gnu with no regressions.

Thanks,
Andrew Pinski

ChangeLog:

        * tree-ssa-sccvn.c (visit_use): Look up the simplified
        expression before visting the original one.

        * gcc.dg/tree-ssa/ssa-fre-9.c: Update the number of
        eliminatations that happen.

[-- Attachment #2: fre_fix.diff.txt --]
[-- Type: text/plain, Size: 1423 bytes --]

Index: gcc/tree-ssa-sccvn.c
===================================================================
--- gcc/tree-ssa-sccvn.c	(revision 189800)
+++ gcc/tree-ssa-sccvn.c	(working copy)
@@ -3366,6 +3366,19 @@ visit_use (tree use)
 		    case GIMPLE_UNARY_RHS:
 		    case GIMPLE_BINARY_RHS:
 		    case GIMPLE_TERNARY_RHS:
+		      /* First try to lookup the simplified expression. */
+		      if (simplified && valid_gimple_rhs_p (simplified))
+			{
+			  tree result = vn_nary_op_lookup (simplified, NULL);
+			  if (result)
+			    {
+			      changed = set_ssa_val_to (lhs, result);
+			      goto done;
+			    }
+			  changed = set_ssa_val_to (lhs, lhs);
+			  vn_nary_op_insert (simplified, lhs);
+			}
+		      /* Otherwise visit the original statement. */
 		      changed = visit_nary_op (lhs, stmt);
 		      break;
 		    case GIMPLE_SINGLE_RHS:
Index: gcc/testsuite/gcc.dg/tree-ssa/ssa-fre-9.c
===================================================================
--- gcc/testsuite/gcc.dg/tree-ssa/ssa-fre-9.c	(revision 189800)
+++ gcc/testsuite/gcc.dg/tree-ssa/ssa-fre-9.c	(working copy)
@@ -23,6 +23,6 @@ void __frame_state_for1 (volatile char *
     }
 }
 
-/* { dg-final { scan-tree-dump-times "Eliminated: 1" 2 "fre1" } } */
+/* { dg-final { scan-tree-dump-times "Eliminated: 2" 2 "fre1" } } */
 /* { dg-final { scan-tree-dump-times "Insertions: 1" 2 "fre1" } } */
 /* { dg-final { cleanup-tree-dump "fre1" } } */

Re: [PATCH] Fix PR 52631 (VN does not use simplified expression for lookup)

[Richard Guenther]

On Tue, Jul 24, 2012 at 5:50 PM, Andrew Pinski
<andrew.pinski@caviumnetworks.com> wrote:
> Hi,
>   Before tuples was introduced, VN used to lookup the simplified
> expression to see if it was available already and use that instead of
> the non simplified one.  This patch adds the support back to VN to do
> exactly that.
>
> OK? Bootstrapped and tested on x86_64-linux-gnu with no regressions.

I think this should be done for all RHS and SSA name LHS, not only
for UNARY/BINARY/TERNARY - because even for SINGLE rhs we
can end up simplifying (for REALPART_EXPR for example which we
handle as nary, too).  I think we constrain try_to_simplify enough
so that

+		      /* First try to lookup the simplified expression. */
+		      if (simplified && valid_gimple_rhs_p (simplified))
+			{
+			  tree result = vn_nary_op_lookup (simplified, NULL);
+			  if (result)
+			    {
+			      changed = set_ssa_val_to (lhs, result);
+			      goto done;
+			    }
+			  changed = set_ssa_val_to (lhs, lhs);
+			  vn_nary_op_insert (simplified, lhs);
+			}
                  switch (get_gimple_rhs_class (code))
                    {
                    case GIMPLE_UNARY_RHS:
                    case GIMPLE_BINARY_RHS:
...

should work.  As you also insert the simplified variant I think we really
(finally) want to have a valid_nary_op routine rather than relying on
valid_gimple_rhs_p which is way too generic.

Thanks,
Richard.

> Thanks,
> Andrew Pinski
>
> ChangeLog:
>
>         * tree-ssa-sccvn.c (visit_use): Look up the simplified
>         expression before visting the original one.
>
>         * gcc.dg/tree-ssa/ssa-fre-9.c: Update the number of
>         eliminatations that happen.

Re: [PATCH] Fix PR 52631 (VN does not use simplified expression for lookup)

[Andrew Pinski]

On Wed, Jul 25, 2012 at 4:39 AM, Richard Guenther
<richard.guenther@gmail.com> wrote:
> On Tue, Jul 24, 2012 at 5:50 PM, Andrew Pinski
> <andrew.pinski@caviumnetworks.com> wrote:
>> Hi,
>>   Before tuples was introduced, VN used to lookup the simplified
>> expression to see if it was available already and use that instead of
>> the non simplified one.  This patch adds the support back to VN to do
>> exactly that.
>>
>> OK? Bootstrapped and tested on x86_64-linux-gnu with no regressions.
>
> I think this should be done for all RHS and SSA name LHS, not only
> for UNARY/BINARY/TERNARY - because even for SINGLE rhs we
> can end up simplifying (for REALPART_EXPR for example which we
> handle as nary, too).  I think we constrain try_to_simplify enough
> so that
>
> +                     /* First try to lookup the simplified expression. */
> +                     if (simplified && valid_gimple_rhs_p (simplified))
> +                       {
> +                         tree result = vn_nary_op_lookup (simplified, NULL);
> +                         if (result)
> +                           {
> +                             changed = set_ssa_val_to (lhs, result);
> +                             goto done;
> +                           }
> +                         changed = set_ssa_val_to (lhs, lhs);
> +                         vn_nary_op_insert (simplified, lhs);
> +                       }
>                   switch (get_gimple_rhs_class (code))
>                     {
>                     case GIMPLE_UNARY_RHS:
>                     case GIMPLE_BINARY_RHS:
> ...
>
> should work.  As you also insert the simplified variant I think we really
> (finally) want to have a valid_nary_op routine rather than relying on
> valid_gimple_rhs_p which is way too generic.

I don't see valid_gimple_rhs_p being that generic as it checks to make
sure the operands of the gimple are valid.
Maybe I am missing something here though.

Thanks,
Andrew Pinski


>
> Thanks,
> Richard.
>
>> Thanks,
>> Andrew Pinski
>>
>> ChangeLog:
>>
>>         * tree-ssa-sccvn.c (visit_use): Look up the simplified
>>         expression before visting the original one.
>>
>>         * gcc.dg/tree-ssa/ssa-fre-9.c: Update the number of
>>         eliminatations that happen.

Re: [PATCH] Fix PR 52631 (VN does not use simplified expression for lookup)

[Richard Guenther]

On Mon, Aug 20, 2012 at 6:49 AM, Andrew Pinski
<andrew.pinski@caviumnetworks.com> wrote:
> On Wed, Jul 25, 2012 at 4:39 AM, Richard Guenther
> <richard.guenther@gmail.com> wrote:
>> On Tue, Jul 24, 2012 at 5:50 PM, Andrew Pinski
>> <andrew.pinski@caviumnetworks.com> wrote:
>>> Hi,
>>>   Before tuples was introduced, VN used to lookup the simplified
>>> expression to see if it was available already and use that instead of
>>> the non simplified one.  This patch adds the support back to VN to do
>>> exactly that.
>>>
>>> OK? Bootstrapped and tested on x86_64-linux-gnu with no regressions.
>>
>> I think this should be done for all RHS and SSA name LHS, not only
>> for UNARY/BINARY/TERNARY - because even for SINGLE rhs we
>> can end up simplifying (for REALPART_EXPR for example which we
>> handle as nary, too).  I think we constrain try_to_simplify enough
>> so that
>>
>> +                     /* First try to lookup the simplified expression. */
>> +                     if (simplified && valid_gimple_rhs_p (simplified))
>> +                       {
>> +                         tree result = vn_nary_op_lookup (simplified, NULL);
>> +                         if (result)
>> +                           {
>> +                             changed = set_ssa_val_to (lhs, result);
>> +                             goto done;
>> +                           }
>> +                         changed = set_ssa_val_to (lhs, lhs);
>> +                         vn_nary_op_insert (simplified, lhs);
>> +                       }
>>                   switch (get_gimple_rhs_class (code))
>>                     {
>>                     case GIMPLE_UNARY_RHS:
>>                     case GIMPLE_BINARY_RHS:
>> ...
>>
>> should work.  As you also insert the simplified variant I think we really
>> (finally) want to have a valid_nary_op routine rather than relying on
>> valid_gimple_rhs_p which is way too generic.
>
> I don't see valid_gimple_rhs_p being that generic as it checks to make
> sure the operands of the gimple are valid.
> Maybe I am missing something here though.

valid_gimple_rhs_p checks what it says.  But what we want to know is whether
the rhs is valid for a SCCVN NARY.  Those are not the same.

Richard.

> Thanks,
> Andrew Pinski
>
>
>>
>> Thanks,
>> Richard.
>>
>>> Thanks,
>>> Andrew Pinski
>>>
>>> ChangeLog:
>>>
>>>         * tree-ssa-sccvn.c (visit_use): Look up the simplified
>>>         expression before visting the original one.
>>>
>>>         * gcc.dg/tree-ssa/ssa-fre-9.c: Update the number of
>>>         eliminatations that happen.