* Re: [PATCH] Enhance PHI processing in VN
@ 2017-09-14 16:51 David Edelsohn
2017-09-15 6:53 ` Richard Biener
0 siblings, 1 reply; 9+ messages in thread
From: David Edelsohn @ 2017-09-14 16:51 UTC (permalink / raw)
To: Richard Biener; +Cc: GCC Patches
* tree-ssa-sccvn.c (visit_phi): Merge undefined values similar
to VN_TOP.
This seems to have regressed
FAIL: gcc.dg/tree-prof/time-profiler-2.c scan-ipa-dump-times profile
"Read tp_first_run: 0" 2
FAIL: gcc.dg/tree-prof/time-profiler-2.c scan-ipa-dump-times profile
"Read tp_first_run: 2" 1
FAIL: gcc.dg/tree-prof/time-profiler-2.c scan-ipa-dump-times profile
"Read tp_first_run: 3" 1
- David
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Enhance PHI processing in VN
2017-09-14 16:51 [PATCH] Enhance PHI processing in VN David Edelsohn
@ 2017-09-15 6:53 ` Richard Biener
2017-09-15 10:40 ` David Edelsohn
0 siblings, 1 reply; 9+ messages in thread
From: Richard Biener @ 2017-09-15 6:53 UTC (permalink / raw)
To: David Edelsohn; +Cc: GCC Patches
On Thu, 14 Sep 2017, David Edelsohn wrote:
> * tree-ssa-sccvn.c (visit_phi): Merge undefined values similar
> to VN_TOP.
>
> This seems to have regressed
>
> FAIL: gcc.dg/tree-prof/time-profiler-2.c scan-ipa-dump-times profile
> "Read tp_first_run: 0" 2
> FAIL: gcc.dg/tree-prof/time-profiler-2.c scan-ipa-dump-times profile
> "Read tp_first_run: 2" 1
> FAIL: gcc.dg/tree-prof/time-profiler-2.c scan-ipa-dump-times profile
> "Read tp_first_run: 3" 1
Hmm, I don't see these FAILs. Looking at the testcase there are
no undefined uses so I wonder how the patch could have any effect.
Can you re-check and open a bugreport?
Thanks,
Richard.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Enhance PHI processing in VN
2017-09-15 6:53 ` Richard Biener
@ 2017-09-15 10:40 ` David Edelsohn
2017-09-27 22:58 ` Richard Sandiford
0 siblings, 1 reply; 9+ messages in thread
From: David Edelsohn @ 2017-09-15 10:40 UTC (permalink / raw)
To: Richard Biener; +Cc: GCC Patches
On Fri, Sep 15, 2017 at 2:53 AM, Richard Biener <rguenther@suse.de> wrote:
> On Thu, 14 Sep 2017, David Edelsohn wrote:
>
>> * tree-ssa-sccvn.c (visit_phi): Merge undefined values similar
>> to VN_TOP.
>>
>> This seems to have regressed
>>
>> FAIL: gcc.dg/tree-prof/time-profiler-2.c scan-ipa-dump-times profile
>> "Read tp_first_run: 0" 2
>> FAIL: gcc.dg/tree-prof/time-profiler-2.c scan-ipa-dump-times profile
>> "Read tp_first_run: 2" 1
>> FAIL: gcc.dg/tree-prof/time-profiler-2.c scan-ipa-dump-times profile
>> "Read tp_first_run: 3" 1
>
> Hmm, I don't see these FAILs. Looking at the testcase there are
> no undefined uses so I wonder how the patch could have any effect.
>
> Can you re-check and open a bugreport?
It disappeared again. A different failure appeared and disappeared a
few weeks ago. Something in the testsuite infrastructure appears to
not be stable, at least on AIX. Sorry for the incorrect report.
- David
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Enhance PHI processing in VN
2017-09-15 10:40 ` David Edelsohn
@ 2017-09-27 22:58 ` Richard Sandiford
2017-09-28 0:03 ` David Edelsohn
0 siblings, 1 reply; 9+ messages in thread
From: Richard Sandiford @ 2017-09-27 22:58 UTC (permalink / raw)
To: David Edelsohn; +Cc: Richard Biener, GCC Patches
David Edelsohn <dje.gcc@gmail.com> writes:
> On Fri, Sep 15, 2017 at 2:53 AM, Richard Biener <rguenther@suse.de> wrote:
>> On Thu, 14 Sep 2017, David Edelsohn wrote:
>>
>>> * tree-ssa-sccvn.c (visit_phi): Merge undefined values similar
>>> to VN_TOP.
>>>
>>> This seems to have regressed
>>>
>>> FAIL: gcc.dg/tree-prof/time-profiler-2.c scan-ipa-dump-times profile
>>> "Read tp_first_run: 0" 2
>>> FAIL: gcc.dg/tree-prof/time-profiler-2.c scan-ipa-dump-times profile
>>> "Read tp_first_run: 2" 1
>>> FAIL: gcc.dg/tree-prof/time-profiler-2.c scan-ipa-dump-times profile
>>> "Read tp_first_run: 3" 1
>>
>> Hmm, I don't see these FAILs. Looking at the testcase there are
>> no undefined uses so I wonder how the patch could have any effect.
>>
>> Can you re-check and open a bugreport?
>
> It disappeared again. A different failure appeared and disappeared a
> few weeks ago. Something in the testsuite infrastructure appears to
> not be stable, at least on AIX. Sorry for the incorrect report.
Perhaps this is unrelated, but when doing the "has this patch
changed assembly on these targets?" testing, I noticed that AIX
had differences like:
--- old/powerpc-ibm-aix7.0/test/-O3/g++.dg/init/constant1.s
+++ new/powerpc-ibm-aix7.0/test/-O3/g++.dg/init/constant1.s
@@ -4,21 +4,21 @@
.csect ..text.startup[PR],2
.align 2
.align 4
- .globl _GLOBAL__I_65535_0_.._.._.._testsuite_g__.dg_._init_constant1.C_00000000_0xbb0d20e181e3a401
- .globl ._GLOBAL__I_65535_0_.._.._.._testsuite_g__.dg_._init_constant1.C_00000000_0xbb0d20e181e3a401
- .csect _GLOBAL__I_65535_0_.._.._.._testsuite_g__.dg_._init_constant1.C_00000000_0xbb0d20e181e3a401[DS]
-_GLOBAL__I_65535_0_.._.._.._testsuite_g__.dg_._init_constant1.C_00000000_0xbb0d20e181e3a401:
- .long ._GLOBAL__I_65535_0_.._.._.._testsuite_g__.dg_._init_constant1.C_00000000_0xbb0d20e181e3a401, TOC[tc0], 0
+ .globl _GLOBAL__I_65535_0_.._.._.._testsuite_g__.dg_._init_constant1.C_00000000_0x5610f7ec143966c9
+ .globl ._GLOBAL__I_65535_0_.._.._.._testsuite_g__.dg_._init_constant1.C_00000000_0x5610f7ec143966c9
+ .csect _GLOBAL__I_65535_0_.._.._.._testsuite_g__.dg_._init_constant1.C_00000000_0x5610f7ec143966c9[DS]
+_GLOBAL__I_65535_0_.._.._.._testsuite_g__.dg_._init_constant1.C_00000000_0x5610f7ec143966c9:
+ .long ._GLOBAL__I_65535_0_.._.._.._testsuite_g__.dg_._init_constant1.C_00000000_0x5610f7ec143966c9, TOC[tc0], 0
even though -frandom-seed is forced to the same value for both runs.
Is this behaviour deliberate? I thought runs from a few weeks ago
had stable names, but maybe I just misremember.
Thanks,
Richard
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Enhance PHI processing in VN
2017-09-27 22:58 ` Richard Sandiford
@ 2017-09-28 0:03 ` David Edelsohn
2017-09-28 19:06 ` Richard Sandiford
0 siblings, 1 reply; 9+ messages in thread
From: David Edelsohn @ 2017-09-28 0:03 UTC (permalink / raw)
To: Richard Biener, GCC Patches, richard.sandiford
On Wed, Sep 27, 2017 at 6:58 PM, Richard Sandiford
<richard.sandiford@linaro.org> wrote:
> David Edelsohn <dje.gcc@gmail.com> writes:
>> On Fri, Sep 15, 2017 at 2:53 AM, Richard Biener <rguenther@suse.de> wrote:
>>> On Thu, 14 Sep 2017, David Edelsohn wrote:
>>>
>>>> * tree-ssa-sccvn.c (visit_phi): Merge undefined values similar
>>>> to VN_TOP.
>>>>
>>>> This seems to have regressed
>>>>
>>>> FAIL: gcc.dg/tree-prof/time-profiler-2.c scan-ipa-dump-times profile
>>>> "Read tp_first_run: 0" 2
>>>> FAIL: gcc.dg/tree-prof/time-profiler-2.c scan-ipa-dump-times profile
>>>> "Read tp_first_run: 2" 1
>>>> FAIL: gcc.dg/tree-prof/time-profiler-2.c scan-ipa-dump-times profile
>>>> "Read tp_first_run: 3" 1
>>>
>>> Hmm, I don't see these FAILs. Looking at the testcase there are
>>> no undefined uses so I wonder how the patch could have any effect.
>>>
>>> Can you re-check and open a bugreport?
>>
>> It disappeared again. A different failure appeared and disappeared a
>> few weeks ago. Something in the testsuite infrastructure appears to
>> not be stable, at least on AIX. Sorry for the incorrect report.
>
> Perhaps this is unrelated, but when doing the "has this patch
> changed assembly on these targets?" testing, I noticed that AIX
> had differences like:
>
> --- old/powerpc-ibm-aix7.0/test/-O3/g++.dg/init/constant1.s
> +++ new/powerpc-ibm-aix7.0/test/-O3/g++.dg/init/constant1.s
> @@ -4,21 +4,21 @@
> .csect ..text.startup[PR],2
> .align 2
> .align 4
> - .globl _GLOBAL__I_65535_0_.._.._.._testsuite_g__.dg_._init_constant1.C_00000000_0xbb0d20e181e3a401
> - .globl ._GLOBAL__I_65535_0_.._.._.._testsuite_g__.dg_._init_constant1.C_00000000_0xbb0d20e181e3a401
> - .csect _GLOBAL__I_65535_0_.._.._.._testsuite_g__.dg_._init_constant1.C_00000000_0xbb0d20e181e3a401[DS]
> -_GLOBAL__I_65535_0_.._.._.._testsuite_g__.dg_._init_constant1.C_00000000_0xbb0d20e181e3a401:
> - .long ._GLOBAL__I_65535_0_.._.._.._testsuite_g__.dg_._init_constant1.C_00000000_0xbb0d20e181e3a401, TOC[tc0], 0
> + .globl _GLOBAL__I_65535_0_.._.._.._testsuite_g__.dg_._init_constant1.C_00000000_0x5610f7ec143966c9
> + .globl ._GLOBAL__I_65535_0_.._.._.._testsuite_g__.dg_._init_constant1.C_00000000_0x5610f7ec143966c9
> + .csect _GLOBAL__I_65535_0_.._.._.._testsuite_g__.dg_._init_constant1.C_00000000_0x5610f7ec143966c9[DS]
> +_GLOBAL__I_65535_0_.._.._.._testsuite_g__.dg_._init_constant1.C_00000000_0x5610f7ec143966c9:
> + .long ._GLOBAL__I_65535_0_.._.._.._testsuite_g__.dg_._init_constant1.C_00000000_0x5610f7ec143966c9, TOC[tc0], 0
>
> even though -frandom-seed is forced to the same value for both runs.
>
> Is this behaviour deliberate? I thought runs from a few weeks ago
> had stable names, but maybe I just misremember.
AIX does not have init/fini sections, so it is built at link time as a
C file by collect2-ld. I suspect that collect2-ld doesn't use
-frandom-seed to build its temporary file.
- David
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Enhance PHI processing in VN
2017-09-28 0:03 ` David Edelsohn
@ 2017-09-28 19:06 ` Richard Sandiford
0 siblings, 0 replies; 9+ messages in thread
From: Richard Sandiford @ 2017-09-28 19:06 UTC (permalink / raw)
To: David Edelsohn; +Cc: Richard Biener, GCC Patches
David Edelsohn <dje.gcc@gmail.com> writes:
> On Wed, Sep 27, 2017 at 6:58 PM, Richard Sandiford
> <richard.sandiford@linaro.org> wrote:
>> David Edelsohn <dje.gcc@gmail.com> writes:
>>> On Fri, Sep 15, 2017 at 2:53 AM, Richard Biener <rguenther@suse.de> wrote:
>>>> On Thu, 14 Sep 2017, David Edelsohn wrote:
>>>>
>>>>> * tree-ssa-sccvn.c (visit_phi): Merge undefined values similar
>>>>> to VN_TOP.
>>>>>
>>>>> This seems to have regressed
>>>>>
>>>>> FAIL: gcc.dg/tree-prof/time-profiler-2.c scan-ipa-dump-times profile
>>>>> "Read tp_first_run: 0" 2
>>>>> FAIL: gcc.dg/tree-prof/time-profiler-2.c scan-ipa-dump-times profile
>>>>> "Read tp_first_run: 2" 1
>>>>> FAIL: gcc.dg/tree-prof/time-profiler-2.c scan-ipa-dump-times profile
>>>>> "Read tp_first_run: 3" 1
>>>>
>>>> Hmm, I don't see these FAILs. Looking at the testcase there are
>>>> no undefined uses so I wonder how the patch could have any effect.
>>>>
>>>> Can you re-check and open a bugreport?
>>>
>>> It disappeared again. A different failure appeared and disappeared a
>>> few weeks ago. Something in the testsuite infrastructure appears to
>>> not be stable, at least on AIX. Sorry for the incorrect report.
>>
>> Perhaps this is unrelated, but when doing the "has this patch
>> changed assembly on these targets?" testing, I noticed that AIX
>> had differences like:
>>
>> --- old/powerpc-ibm-aix7.0/test/-O3/g++.dg/init/constant1.s
>> +++ new/powerpc-ibm-aix7.0/test/-O3/g++.dg/init/constant1.s
>> @@ -4,21 +4,21 @@
>> .csect ..text.startup[PR],2
>> .align 2
>> .align 4
>> - .globl _GLOBAL__I_65535_0_.._.._.._testsuite_g__.dg_._init_constant1.C_00000000_0xbb0d20e181e3a401
>> - .globl ._GLOBAL__I_65535_0_.._.._.._testsuite_g__.dg_._init_constant1.C_00000000_0xbb0d20e181e3a401
>> - .csect _GLOBAL__I_65535_0_.._.._.._testsuite_g__.dg_._init_constant1.C_00000000_0xbb0d20e181e3a401[DS]
>> -_GLOBAL__I_65535_0_.._.._.._testsuite_g__.dg_._init_constant1.C_00000000_0xbb0d20e181e3a401:
>> - .long ._GLOBAL__I_65535_0_.._.._.._testsuite_g__.dg_._init_constant1.C_00000000_0xbb0d20e181e3a401, TOC[tc0], 0
>> + .globl _GLOBAL__I_65535_0_.._.._.._testsuite_g__.dg_._init_constant1.C_00000000_0x5610f7ec143966c9
>> + .globl ._GLOBAL__I_65535_0_.._.._.._testsuite_g__.dg_._init_constant1.C_00000000_0x5610f7ec143966c9
>> + .csect _GLOBAL__I_65535_0_.._.._.._testsuite_g__.dg_._init_constant1.C_00000000_0x5610f7ec143966c9[DS]
>> +_GLOBAL__I_65535_0_.._.._.._testsuite_g__.dg_._init_constant1.C_00000000_0x5610f7ec143966c9:
>> + .long ._GLOBAL__I_65535_0_.._.._.._testsuite_g__.dg_._init_constant1.C_00000000_0x5610f7ec143966c9, TOC[tc0], 0
>>
>> even though -frandom-seed is forced to the same value for both runs.
>>
>> Is this behaviour deliberate? I thought runs from a few weeks ago
>> had stable names, but maybe I just misremember.
>
> AIX does not have init/fini sections, so it is built at link time as a
> C file by collect2-ld. I suspect that collect2-ld doesn't use
> -frandom-seed to build its temporary file.
This is just comparing -S output, so the difference seems to be in
the compiler itself.
Thanks,
Richard
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Enhance PHI processing in VN
2017-09-07 9:20 ` Richard Biener
@ 2017-09-14 11:56 ` Richard Biener
0 siblings, 0 replies; 9+ messages in thread
From: Richard Biener @ 2017-09-14 11:56 UTC (permalink / raw)
To: gcc-patches
On Thu, 7 Sep 2017, Richard Biener wrote:
> On Thu, 7 Sep 2017, Richard Biener wrote:
>
> >
> > This enhances VN to do the same PHI handling as CCP, meeting
> > undefined and constant to constant. I've gone a little bit
> > further (and maybe will revisit this again) in also meeting
> > all-undefined to undefined taking one of the undefined args
> > as the value number. I feel like this might run into
> > the equation issues I mentioned in the other mail so it
> > would be cleaner to invent a "new" undefined value number
> > here -- but I have to make sure to not create too many
> > default-defs or break iteration convergence (default defs are also
> > expensive given they require a decl - sth I want to change as well).
> >
> > So for now I guess I'll stick with the slightly bogus(?) way also
> > hoping for a testcase that shows the issue this uncovers.
> >
> > Note it's required to handle
> >
> > _3 = PHI <_1(D), _2(D)>
> > ..
> >
> > _4 = PHI <_3, 1>
> >
> > consistently with
> >
> > _4 = PHI <_1(D), _2(D), 1>
> >
> > aka with/without extra forwarders.
>
> That said, "fallout" is we simplify
>
> int foo (int b)
> {
> int i, j, k;
> if (b)
> k = i;
> else
> k = j;
> if (k == i)
> return 1;
> else if (k == j)
> return 2;
> return 0;
>
> to
>
> if (j == i)
> return 1;
> else
> return 2;
>
> or even just
>
> return 2;
>
> dependent on PHI argument order of k = PHI <i(D), j(D)>.
>
> Likewise we'd say that either k - i or k - j is zero.
>
> The complication with PHIs is that they do not always only appear
> in places where uses of the args dominate it but the other way
> around so we can't really invoke the undefined behavior rule
> on a PHI node with undefined args itself. The question is whether
> we may for PHIs with just undefined args ... but my guess is no
> so I do have to fix the above.
>
> Anybody can produce a testcase that we'd consider wrong-code?
> (the above examples clearly are not)
After some pondering I decided for PHIs with all undefs there's
really no way things can go wrong.
Thus the following is what I installed.
Bootstrapped and tested on x86_64-unknown-linux-gnu.
Richard.
2017-09-14 Richard Biener <rguenther@suse.de>
* tree-ssa-sccvn.c (visit_phi): Merge undefined values similar
to VN_TOP.
* gcc.dg/tree-ssa/ssa-fre-59.c: New testcase.
* gcc.dg/uninit-suppress_2.c: Adjust.
* gcc.dg/tree-ssa/ssa-sccvn-2.c: Likewise.
Index: gcc/tree-ssa-sccvn.c
===================================================================
--- gcc/tree-ssa-sccvn.c (revision 252062)
+++ gcc/tree-ssa-sccvn.c (working copy)
@@ -3860,11 +3860,11 @@ visit_reference_op_store (tree lhs, tree
static bool
visit_phi (gimple *phi)
{
- bool changed = false;
- tree result;
- tree sameval = VN_TOP;
- bool allsame = true;
+ tree result, sameval = VN_TOP, seen_undef = NULL_TREE;
unsigned n_executable = 0;
+ bool allsame = true;
+ edge_iterator ei;
+ edge e;
/* TODO: We could check for this in init_sccvn, and replace this
with a gcc_assert. */
@@ -3873,8 +3873,6 @@ visit_phi (gimple *phi)
/* See if all non-TOP arguments have the same value. TOP is
equivalent to everything, so we can ignore it. */
- edge_iterator ei;
- edge e;
FOR_EACH_EDGE (e, ei, gimple_bb (phi)->preds)
if (e->flags & EDGE_EXECUTABLE)
{
@@ -3884,8 +3882,12 @@ visit_phi (gimple *phi)
if (TREE_CODE (def) == SSA_NAME)
def = SSA_VAL (def);
if (def == VN_TOP)
- continue;
- if (sameval == VN_TOP)
+ ;
+ /* Ignore undefined defs for sameval but record one. */
+ else if (TREE_CODE (def) == SSA_NAME
+ && ssa_undefined_value_p (def, false))
+ seen_undef = def;
+ else if (sameval == VN_TOP)
sameval = def;
else if (!expressions_equal_p (def, sameval))
{
@@ -3893,30 +3895,39 @@ visit_phi (gimple *phi)
break;
}
}
-
- /* If none of the edges was executable or all incoming values are
- undefined keep the value-number at VN_TOP. If only a single edge
- is exectuable use its value. */
- if (sameval == VN_TOP
- || n_executable == 1)
- return set_ssa_val_to (PHI_RESULT (phi), sameval);
+
+ /* If none of the edges was executable keep the value-number at VN_TOP,
+ if only a single edge is exectuable use its value. */
+ if (n_executable <= 1)
+ result = seen_undef ? seen_undef : sameval;
+ /* If we saw only undefined values create a new undef SSA name to
+ avoid false equivalences. */
+ else if (sameval == VN_TOP)
+ {
+ gcc_assert (seen_undef);
+ result = seen_undef;
+ }
/* First see if it is equivalent to a phi node in this block. We prefer
this as it allows IV elimination - see PRs 66502 and 67167. */
- result = vn_phi_lookup (phi);
- if (result)
- changed = set_ssa_val_to (PHI_RESULT (phi), result);
- /* Otherwise all value numbered to the same value, the phi node has that
- value. */
- else if (allsame)
- changed = set_ssa_val_to (PHI_RESULT (phi), sameval);
+ else if ((result = vn_phi_lookup (phi)))
+ ;
+ /* If all values are the same use that, unless we've seen undefined
+ values as well and the value isn't constant.
+ CCP/copyprop have the same restriction to not remove uninit warnings. */
+ else if (allsame
+ && (! seen_undef || is_gimple_min_invariant (sameval)))
+ result = sameval;
else
{
- vn_phi_insert (phi, PHI_RESULT (phi));
- changed = set_ssa_val_to (PHI_RESULT (phi), PHI_RESULT (phi));
+ result = PHI_RESULT (phi);
+ /* Only insert PHIs that are varying, for constant value numbers
+ we mess up equivalences otherwise as we are only comparing
+ the immediate controlling predicates. */
+ vn_phi_insert (phi, result);
}
- return changed;
+ return set_ssa_val_to (PHI_RESULT (phi), result);
}
/* Try to simplify RHS using equivalences and constant folding. */
Index: gcc/testsuite/gcc.dg/tree-ssa/ssa-fre-59.c
===================================================================
--- gcc/testsuite/gcc.dg/tree-ssa/ssa-fre-59.c (nonexistent)
+++ gcc/testsuite/gcc.dg/tree-ssa/ssa-fre-59.c (working copy)
@@ -0,0 +1,14 @@
+/* { dg-do compile } */
+/* { dg-options "-O -fdump-tree-fre1" } */
+
+int i;
+int foo (int b)
+{
+ int j;
+ i = 1;
+ if (b)
+ j = i;
+ return i - j;
+}
+
+/* { dg-final { scan-tree-dump "return 0;" "fre1" } } */
Index: gcc/testsuite/gcc.dg/uninit-suppress_2.c
===================================================================
--- gcc/testsuite/gcc.dg/uninit-suppress_2.c (revision 252062)
+++ gcc/testsuite/gcc.dg/uninit-suppress_2.c (working copy)
@@ -1,5 +1,5 @@
/* { dg-do compile } */
-/* { dg-options "-fno-tree-ccp -fno-tree-vrp -O2 -Wuninitialized -Werror=uninitialized -Wno-error=maybe-uninitialized" } */
+/* { dg-options "-fno-tree-ccp -fno-tree-vrp -fno-tree-fre -fno-tree-pre -fno-code-hoisting -O2 -Wuninitialized -Werror=uninitialized -Wno-error=maybe-uninitialized" } */
void blah();
void bar (int);
int gflag;
Index: gcc/testsuite/gcc.dg/tree-ssa/ssa-sccvn-2.c
===================================================================
--- gcc/testsuite/gcc.dg/tree-ssa/ssa-sccvn-2.c (revision 252062)
+++ gcc/testsuite/gcc.dg/tree-ssa/ssa-sccvn-2.c (working copy)
@@ -21,4 +21,4 @@ int vnum_test8(int *data)
}
/* We should eliminate m - n, and set n = n + k into n = m, and
set p to 0 */
-/* { dg-final { scan-tree-dump-times "Eliminated: 4" 1 "fre1"} } */
+/* { dg-final { scan-tree-dump-times "Eliminated: 5" 1 "fre1"} } */
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Enhance PHI processing in VN
2017-09-07 8:46 Richard Biener
@ 2017-09-07 9:20 ` Richard Biener
2017-09-14 11:56 ` Richard Biener
0 siblings, 1 reply; 9+ messages in thread
From: Richard Biener @ 2017-09-07 9:20 UTC (permalink / raw)
To: gcc-patches
On Thu, 7 Sep 2017, Richard Biener wrote:
>
> This enhances VN to do the same PHI handling as CCP, meeting
> undefined and constant to constant. I've gone a little bit
> further (and maybe will revisit this again) in also meeting
> all-undefined to undefined taking one of the undefined args
> as the value number. I feel like this might run into
> the equation issues I mentioned in the other mail so it
> would be cleaner to invent a "new" undefined value number
> here -- but I have to make sure to not create too many
> default-defs or break iteration convergence (default defs are also
> expensive given they require a decl - sth I want to change as well).
>
> So for now I guess I'll stick with the slightly bogus(?) way also
> hoping for a testcase that shows the issue this uncovers.
>
> Note it's required to handle
>
> _3 = PHI <_1(D), _2(D)>
> ..
>
> _4 = PHI <_3, 1>
>
> consistently with
>
> _4 = PHI <_1(D), _2(D), 1>
>
> aka with/without extra forwarders.
That said, "fallout" is we simplify
int foo (int b)
{
int i, j, k;
if (b)
k = i;
else
k = j;
if (k == i)
return 1;
else if (k == j)
return 2;
return 0;
to
if (j == i)
return 1;
else
return 2;
or even just
return 2;
dependent on PHI argument order of k = PHI <i(D), j(D)>.
Likewise we'd say that either k - i or k - j is zero.
The complication with PHIs is that they do not always only appear
in places where uses of the args dominate it but the other way
around so we can't really invoke the undefined behavior rule
on a PHI node with undefined args itself. The question is whether
we may for PHIs with just undefined args ... but my guess is no
so I do have to fix the above.
Anybody can produce a testcase that we'd consider wrong-code?
(the above examples clearly are not)
Thanks,
Richard.
> Similar to CCP/copyprop this doesn't allow copies to be propagated
> this way as this removes most gcc.dg/Wuninitialized* warnings
> we test for...
>
> Bootstrapped on x86_64-unknown-linux-gnu, testing in progress.
>
> Richard.
>
> 2017-09-07 Richard Biener <rguenther@suse.de>
>
> * tree-ssa-sccnv.c (visit_phi): Handle meeting undefined
> values.
>
> * gcc.dg/tree-ssa/ssa-fre-59.c: New testcase.
>
> Index: gcc/testsuite/gcc.dg/tree-ssa/ssa-fre-59.c
> ===================================================================
> --- gcc/testsuite/gcc.dg/tree-ssa/ssa-fre-59.c (nonexistent)
> +++ gcc/testsuite/gcc.dg/tree-ssa/ssa-fre-59.c (working copy)
> @@ -0,0 +1,14 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O -fdump-tree-fre1" } */
> +
> +int i;
> +int foo (int b)
> +{
> + int j;
> + i = 1;
> + if (b)
> + j = i;
> + return i - j;
> +}
> +
> +/* { dg-final { scan-tree-dump "return 0;" "fre1" } } */
> Index: gcc/tree-ssa-sccvn.c
> ===================================================================
> --- gcc/tree-ssa-sccvn.c (revision 251833)
> +++ gcc/tree-ssa-sccvn.c (working copy)
> @@ -3864,6 +3864,7 @@ visit_phi (gimple *phi)
> tree result;
> tree sameval = VN_TOP;
> bool allsame = true;
> + tree seen_undef = NULL_TREE;
> unsigned n_executable = 0;
>
> /* TODO: We could check for this in init_sccvn, and replace this
> @@ -3884,8 +3885,12 @@ visit_phi (gimple *phi)
> if (TREE_CODE (def) == SSA_NAME)
> def = SSA_VAL (def);
> if (def == VN_TOP)
> - continue;
> - if (sameval == VN_TOP)
> + ;
> + /* Ignore undefined defs here. */
> + else if (TREE_CODE (def) == SSA_NAME
> + && ssa_undefined_value_p (def, false))
> + seen_undef = def;
> + else if (sameval == VN_TOP)
> sameval = def;
> else if (!expressions_equal_p (def, sameval))
> {
> @@ -3893,7 +3898,18 @@ visit_phi (gimple *phi)
> break;
> }
> }
> -
> + /* If we found undefined values and didn't end up with a constant
> + drop back to varying as otherwise we end up removing most late
> + uninitialized use warnings. CCP/copyprop have the same restriction. */
> + if (allsame && seen_undef)
> + {
> + /* Unless this was the only value we've seen. */
> + if (sameval == VN_TOP)
> + sameval = seen_undef;
> + else if (! is_gimple_min_invariant (sameval))
> + allsame = false;
> + }
> +
> /* If none of the edges was executable or all incoming values are
> undefined keep the value-number at VN_TOP. If only a single edge
> is exectuable use its value. */
>
--
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] Enhance PHI processing in VN
@ 2017-09-07 8:46 Richard Biener
2017-09-07 9:20 ` Richard Biener
0 siblings, 1 reply; 9+ messages in thread
From: Richard Biener @ 2017-09-07 8:46 UTC (permalink / raw)
To: gcc-patches
This enhances VN to do the same PHI handling as CCP, meeting
undefined and constant to constant. I've gone a little bit
further (and maybe will revisit this again) in also meeting
all-undefined to undefined taking one of the undefined args
as the value number. I feel like this might run into
the equation issues I mentioned in the other mail so it
would be cleaner to invent a "new" undefined value number
here -- but I have to make sure to not create too many
default-defs or break iteration convergence (default defs are also
expensive given they require a decl - sth I want to change as well).
So for now I guess I'll stick with the slightly bogus(?) way also
hoping for a testcase that shows the issue this uncovers.
Note it's required to handle
_3 = PHI <_1(D), _2(D)>
..
_4 = PHI <_3, 1>
consistently with
_4 = PHI <_1(D), _2(D), 1>
aka with/without extra forwarders.
Similar to CCP/copyprop this doesn't allow copies to be propagated
this way as this removes most gcc.dg/Wuninitialized* warnings
we test for...
Bootstrapped on x86_64-unknown-linux-gnu, testing in progress.
Richard.
2017-09-07 Richard Biener <rguenther@suse.de>
* tree-ssa-sccnv.c (visit_phi): Handle meeting undefined
values.
* gcc.dg/tree-ssa/ssa-fre-59.c: New testcase.
Index: gcc/testsuite/gcc.dg/tree-ssa/ssa-fre-59.c
===================================================================
--- gcc/testsuite/gcc.dg/tree-ssa/ssa-fre-59.c (nonexistent)
+++ gcc/testsuite/gcc.dg/tree-ssa/ssa-fre-59.c (working copy)
@@ -0,0 +1,14 @@
+/* { dg-do compile } */
+/* { dg-options "-O -fdump-tree-fre1" } */
+
+int i;
+int foo (int b)
+{
+ int j;
+ i = 1;
+ if (b)
+ j = i;
+ return i - j;
+}
+
+/* { dg-final { scan-tree-dump "return 0;" "fre1" } } */
Index: gcc/tree-ssa-sccvn.c
===================================================================
--- gcc/tree-ssa-sccvn.c (revision 251833)
+++ gcc/tree-ssa-sccvn.c (working copy)
@@ -3864,6 +3864,7 @@ visit_phi (gimple *phi)
tree result;
tree sameval = VN_TOP;
bool allsame = true;
+ tree seen_undef = NULL_TREE;
unsigned n_executable = 0;
/* TODO: We could check for this in init_sccvn, and replace this
@@ -3884,8 +3885,12 @@ visit_phi (gimple *phi)
if (TREE_CODE (def) == SSA_NAME)
def = SSA_VAL (def);
if (def == VN_TOP)
- continue;
- if (sameval == VN_TOP)
+ ;
+ /* Ignore undefined defs here. */
+ else if (TREE_CODE (def) == SSA_NAME
+ && ssa_undefined_value_p (def, false))
+ seen_undef = def;
+ else if (sameval == VN_TOP)
sameval = def;
else if (!expressions_equal_p (def, sameval))
{
@@ -3893,7 +3898,18 @@ visit_phi (gimple *phi)
break;
}
}
-
+ /* If we found undefined values and didn't end up with a constant
+ drop back to varying as otherwise we end up removing most late
+ uninitialized use warnings. CCP/copyprop have the same restriction. */
+ if (allsame && seen_undef)
+ {
+ /* Unless this was the only value we've seen. */
+ if (sameval == VN_TOP)
+ sameval = seen_undef;
+ else if (! is_gimple_min_invariant (sameval))
+ allsame = false;
+ }
+
/* If none of the edges was executable or all incoming values are
undefined keep the value-number at VN_TOP. If only a single edge
is exectuable use its value. */
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2017-09-28 19:06 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-09-14 16:51 [PATCH] Enhance PHI processing in VN David Edelsohn
2017-09-15 6:53 ` Richard Biener
2017-09-15 10:40 ` David Edelsohn
2017-09-27 22:58 ` Richard Sandiford
2017-09-28 0:03 ` David Edelsohn
2017-09-28 19:06 ` Richard Sandiford
-- strict thread matches above, loose matches on Subject: below --
2017-09-07 8:46 Richard Biener
2017-09-07 9:20 ` Richard Biener
2017-09-14 11:56 ` Richard Biener
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).