Re: [PATCH] Fix for PR51879 - Missed tail merging with non-const/pure calls

[Richard Guenther <richard dot guenther at gmail dot com>]

On Mon, Jan 23, 2012 at 10:27 PM, Tom de Vries <Tom_deVries@mentor.com> wrote:
> Richard,
> Jakub,
>
> the following patch fixes PR51879.
>
> Consider the following test-case:
> ...
> int bar (int);
> void baz (int);
>
> void
> foo (int y)
> {
> ?int a;
> ?if (y == 6)
> ? ?a = bar (7);
> ?else
> ? ?a = bar (7);
> ?baz (a);
> }
> ...
>
> after compiling at -02, the representation looks like this before tail-merging:
> ...
> ?# BLOCK 3 freq:1991
> ?# PRED: 2 [19.9%] ?(true,exec)
> ?# .MEMD.1714_7 = VDEF <.MEMD.1714_6(D)>
> ?# USE = nonlocal
> ?# CLB = nonlocal
> ?aD.1709_3 = barD.1703 (7);
> ?goto <bb 5>;
> ?# SUCC: 5 [100.0%] ?(fallthru,exec)
>
> ?# BLOCK 4 freq:8009
> ?# PRED: 2 [80.1%] ?(false,exec)
> ?# .MEMD.1714_8 = VDEF <.MEMD.1714_6(D)>
> ?# USE = nonlocal
> ?# CLB = nonlocal
> ?aD.1709_4 = barD.1703 (7);
> ?# SUCC: 5 [100.0%] ?(fallthru,exec)
>
> ?# BLOCK 5 freq:10000
> ?# PRED: 3 [100.0%] ?(fallthru,exec) 4 [100.0%] ?(fallthru,exec)
> ?# aD.1709_1 = PHI <aD.1709_3(3), aD.1709_4(4)>
> ?# .MEMD.1714_5 = PHI <.MEMD.1714_7(3), .MEMD.1714_8(4)>
> ?# .MEMD.1714_9 = VDEF <.MEMD.1714_5>
> ?# USE = nonlocal
> ?# CLB = nonlocal
> ?bazD.1705 (aD.1709_1);
> ?# VUSE <.MEMD.1714_9>
> ?return;
> ...
>
> the patch allows aD.1709_4 to be value numbered to aD.1709_3, and .MEMD.1714_8
> to .MEMD.1714_7, which enables tail-merging of blocks 4 and 5.
>
> The patch makes sure non-const/pure call results (gimple_vdef and
> gimple_call_lhs) are properly value numbered.
>
> Bootstrapped and reg-tested on x86_64.
>
> ok for stage1?

The following cannot really work:

@@ -2600,7 +2601,11 @@ visit_reference_op_call (tree lhs, gimpl
   result = vn_reference_lookup_1 (&vr1, NULL);
   if (result)
     {
-      changed = set_ssa_val_to (lhs, result);
+      tree result_vdef = gimple_vdef (SSA_NAME_DEF_STMT (result));
+      if (vdef)
+       changed |= set_ssa_val_to (vdef, result_vdef);
+      changed |= set_ssa_val_to (lhs, result);

becase 'result' may be not an SSA name.  It might also not have
a proper definition statement (if VN_INFO (result)->needs_insertion
is true).  So you at least need to guard things properly.

(On a side-note - I _did_ want to remove value-numbering virtual operands
at some point ...)

@@ -3359,8 +3366,10 @@ visit_use (tree use)
          /* ???  We should handle stores from calls.  */
          else if (TREE_CODE (lhs) == SSA_NAME)
            {
+             tree vuse = gimple_vuse (stmt);
              if (!gimple_call_internal_p (stmt)
-                 && gimple_call_flags (stmt) & (ECF_PURE | ECF_CONST))
+                 && (gimple_call_flags (stmt) & (ECF_PURE | ECF_CONST)
+                     || (vuse && SSA_VAL (vuse) != VN_TOP)))
                changed = visit_reference_op_call (lhs, stmt);
              else
                changed = defs_to_varying (stmt);

... exactly because of the issue that a stmt has multiple defs.  Btw,
vuse should have been visited here or be part of our SCC, so, why do
you need this check?

Thanks,
Richard.

> Thanks,
> - Tom
>
> 2012-01-23 ?Tom de Vries ?<tom@codesourcery.com>
>
> ? ? ? ?PR tree-optimization/51879
> ? ? ? ?tree-ssa-sccvn.c (visit_reference_op_call): Handle gimple_vdef.
> ? ? ? ?(visit_use): Handle non-pure/const calls using visit_reference_op_call.
>
> ? ? ? ?gcc.dg/pr51879.c: New test.
> ? ? ? ?gcc.dg/pr51879-2.c: Same.
> ? ? ? ?gcc.dg/pr51879-3.c: Same.
> ? ? ? ?gcc.dg/pr51879-4.c: Same.