[Bug rtl-optimization/42839] [4.5 Regression] gcc.target/mips/octeon-bbit-2.c failing for -mabi=64
--- Comment #15 from rguenth at gcc dot gnu dot org 2010-04-06 11:20 --- GCC 4.5.0 is being released. Deferring to 4.5.1. -- rguenth at gcc dot gnu dot org changed: What|Removed |Added Target Milestone|4.5.0 |4.5.1 http://gcc.gnu.org/bugzilla/show_bug.cgi?id=42839
[Bug rtl-optimization/42839] [4.5 Regression] gcc.target/mips/octeon-bbit-2.c failing for -mabi=64
-- mmitchel at gcc dot gnu dot org changed: What|Removed |Added Priority|P3 |P2 http://gcc.gnu.org/bugzilla/show_bug.cgi?id=42839
[Bug rtl-optimization/42839] [4.5 Regression] gcc.target/mips/octeon-bbit-2.c failing for -mabi=64
--- Comment #8 from rguenth at gcc dot gnu dot org 2010-02-13 10:11 --- (In reply to comment #3) Basically yet another example of why it is a REALLY BAD IDEA to use constants as PHI arguments. If the constant from s =0 would not be propagated into the PHI, the statement would not be dead and removed, and no new s=0 would have to be inserted in the wrong place. See bug 42906 for another example of exactly the same problem. Allowing constants as PHI arguments is GCC's biggest design f*ck-up. I'm not sure - do other compilers usually not allow this? It shouldn't be hard to at least disable propagation of constants into PHIs (there may be a number of optimizers generating PHIs with constants initially though). Note that I also see bb 4: # s_1 = PHI s_6(3) again and again - shouldn't these be plain assignments as well? Thus, should blocks with a single predecessor have PHIs at all? I suppose with the insertion at the wrong place you mean bb 3: # i_2 = PHI 0(2) # s_11 = PHI 0(2) as done by the pre-header generated by loop init? (they are dead anyway, but re-generated all the time) -- rguenth at gcc dot gnu dot org changed: What|Removed |Added CC||rguenth at gcc dot gnu dot ||org http://gcc.gnu.org/bugzilla/show_bug.cgi?id=42839
[Bug rtl-optimization/42839] [4.5 Regression] gcc.target/mips/octeon-bbit-2.c failing for -mabi=64
--- Comment #9 from steven at gcc dot gnu dot org 2010-02-13 11:28 --- Re. comment #8: No other compiler, I have ever seen, allows constants as PHI args. Single-argument PHIs should be propagated out. Do you see this in one of the loop passes, then it's OK because they are probably there for loop-closed SSA. Anywhere else, they should be propagated out. We even used to do this in a simple CFG cleanup. It looks like something broke this. With inserting in the wrong place I mean that insert the new s = 0 in a different place from where it was originally (which was the optimal place to begin with). We have pessimized the original code. -- http://gcc.gnu.org/bugzilla/show_bug.cgi?id=42839
[Bug rtl-optimization/42839] [4.5 Regression] gcc.target/mips/octeon-bbit-2.c failing for -mabi=64
--- Comment #10 from steven at gcc dot gnu dot org 2010-02-13 11:39 --- In the test case of comment #2, the history of the funny PHIs is really odd. At -O2 we end with this in the .optimized dump: bb 3: # i_1 = PHI 0(2) # s_11 = PHI 0(2) bb 4: # i_12 = PHI i_1(3), i_7(4) # s_13 = PHI s_11(3), s_6(4) But the history of the phi of i_12 looks like this: t.c.120t.reassoc2: # i_12 = PHI i_7(3), 0(2) t.c.121t.vrp2: # i_12 = PHI i_2(3), i_7(7) t.c.121t.vrp2: # i_12 = PHI 0(3), i_7(4) t.c.122t.dom2: # i_12 = PHI 0(3), i_7(4) t.c.123t.phicprop2: # i_12 = PHI 0(2), i_7(3) t.c.124t.cddce2: # i_12 = PHI i_1(3), i_7(6) Somehow we have re-introduced the special PHI # i_1 = PHI 0(2) in the .cddce2 pass. The reason is that we created a loop pre-header. -- http://gcc.gnu.org/bugzilla/show_bug.cgi?id=42839
[Bug rtl-optimization/42839] [4.5 Regression] gcc.target/mips/octeon-bbit-2.c failing for -mabi=64
--- Comment #11 from rguenther at suse dot de 2010-02-13 12:07 ---
Subject: Re: [4.5 Regression] gcc.target/mips/octeon-bbit-2.c
failing for -mabi=64
On Sat, 13 Feb 2010, steven at gcc dot gnu dot org wrote:
--- Comment #10 from steven at gcc dot gnu dot org 2010-02-13 11:39
---
In the test case of comment #2, the history of the funny PHIs is really odd.
At
-O2 we end with this in the .optimized dump:
bb 3:
# i_1 = PHI 0(2)
# s_11 = PHI 0(2)
bb 4:
# i_12 = PHI i_1(3), i_7(4)
# s_13 = PHI s_11(3), s_6(4)
But the history of the phi of i_12 looks like this:
t.c.120t.reassoc2: # i_12 = PHI i_7(3), 0(2)
t.c.121t.vrp2: # i_12 = PHI i_2(3), i_7(7)
t.c.121t.vrp2: # i_12 = PHI 0(3), i_7(4)
t.c.122t.dom2: # i_12 = PHI 0(3), i_7(4)
t.c.123t.phicprop2: # i_12 = PHI 0(2), i_7(3)
t.c.124t.cddce2: # i_12 = PHI i_1(3), i_7(6)
Somehow we have re-introduced the special PHI # i_1 = PHI 0(2) in the
.cddce2 pass. The reason is that we created a loop pre-header.
Yep. I wonder if cfgcloop should just not do that, and of course
I wonder why cfgcleanup doesn't get rid of single-arg PHIs (well,
probably to not break loop-closed SSA form).
Btw, the following should avoid almost all constant propagations
into PHIs (well, I guess DOM needs fixing as well)
Index: tree-ssa-propagate.c
===
--- tree-ssa-propagate.c(revision 15)
+++ tree-ssa-propagate.c(working copy)
@@ -924,7 +924,9 @@ replace_phi_args_in (gimple phi, prop_va
{
tree val = prop_value[SSA_NAME_VERSION (arg)].value;
- if (val val != arg may_propagate_copy (arg, val))
+ if (val val != arg
+ TREE_CODE (val) == SSA_NAME
+ may_propagate_copy (arg, val))
{
if (TREE_CODE (val) != SSA_NAME)
prop_stats.num_const_prop++;
--
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=42839
[Bug rtl-optimization/42839] [4.5 Regression] gcc.target/mips/octeon-bbit-2.c failing for -mabi=64
--- Comment #12 from rguenth at gcc dot gnu dot org 2010-02-13 12:18
---
Yep.
Index: tree-ssa-dom.c
===
--- tree-ssa-dom.c (revision 15)
+++ tree-ssa-dom.c (working copy)
@@ -1455,8 +1455,7 @@ cprop_into_successor_phis (basic_block b
new_val = SSA_NAME_VALUE (orig_val);
if (new_val
new_val != orig_val
- (TREE_CODE (new_val) == SSA_NAME
- || is_gimple_min_invariant (new_val))
+ TREE_CODE (new_val) == SSA_NAME
may_propagate_copy (orig_val, new_val))
propagate_value (orig_p, new_val);
}
with that we end up with
g (int n, int i)
{
int s;
bb 2:
s_3 = 0;
if (n_5(D) 0)
goto bb 3;
else
goto bb 5;
bb 3:
i_4 = 0;
bb 4:
# i_12 = PHI i_7(4), i_4(3)
# s_13 = PHI s_6(4), s_3(3)
s_6 = s_13 + i_12;
i_7 = i_12 + 1;
if (i_7 != n_5(D))
goto bb 4;
else
goto bb 5;
bb 5:
# s_9 = PHI s_6(4), s_3(2)
return s_9;
}
tree-ssa-sink sunk i_4 = 0 from bb2 to bb3.
Note that this shows that we may have extra BBs without constants in
PHI nodes.
But if it is more sane to not allow this we can experiment with this
for 4.6 quite easily.
--
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=42839
[Bug rtl-optimization/42839] [4.5 Regression] gcc.target/mips/octeon-bbit-2.c failing for -mabi=64
--- Comment #13 from rguenth at gcc dot gnu dot org 2010-02-13 12:19 --- But it has a load of fallout already in the gcc.dg/tree-ssa testsuite parts. -- http://gcc.gnu.org/bugzilla/show_bug.cgi?id=42839
[Bug rtl-optimization/42839] [4.5 Regression] gcc.target/mips/octeon-bbit-2.c failing for -mabi=64
--- Comment #14 from steven at gcc dot gnu dot org 2010-02-13 12:44 --- Re. comment #12, if you mean the extra BB3, that one is not really extra, it comes from loop header copying, and it easy to clean up. I assume ifcvt cleans this up? Re. comment #12, yes that is expected, but most of those failures are easily solved. The bottom line IMHO is this: * propagating constants into PHI args sometimes exposes things, but it conceals more and does irreparable damage * not propagating means we have to work harder in a few places (following SSA_NAME_DEF_STMT in a few places) to avoid missing optimizations, but at least we never make the code worse than the original code (and aoliva would add that we get debug info right if we don't propagate...) -- http://gcc.gnu.org/bugzilla/show_bug.cgi?id=42839
[Bug rtl-optimization/42839] [4.5 Regression] gcc.target/mips/octeon-bbit-2.c failing for -mabi=64
--- Comment #2 from pinskia at gcc dot gnu dot org 2010-02-12 22:12 ---
The problem shows up at the tree level and nothing cleans it up after expand
for MIPS.
Take:
int
g (int n, int i)
{
int s = 0;
for (i = 0; i n; i++)
s += i;
return s;
}
--- CUT ---
For the above code expand on x86_64-linux-gnu produces (I simplified the
branches though but otherwise it is the same as what expand produces):
r64 = %edi # n
r65 = %esi #i
if ( r64 = 0 ) goto L37
r61 = 0
r62 = 0
L19:
r61 = r61 + r62
r62 = r62 + 1
if ( r62 != r64 ) goto L19
goto L22
L37:
r61 = 0
L22:
r63 = r61
goto L27
L27:
%eax = r63
return
-- CUT ---
Notice how we have r61 = 0 which could be moved before the first branch which
allows us to remove a BB.
For x86_64, ce1 moves the r61 above the first branch but does not do it for
mips.
--
pinskia at gcc dot gnu dot org changed:
What|Removed |Added
Component|target |rtl-optimization
GCC build triplet|*-*-* |
GCC host triplet|*-*-* |
GCC target triplet|mips64-unknown-linux|
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=42839
[Bug rtl-optimization/42839] [4.5 Regression] gcc.target/mips/octeon-bbit-2.c failing for -mabi=64
--- Comment #3 from steven at gcc dot gnu dot org 2010-02-12 22:27 --- Basically yet another example of why it is a REALLY BAD IDEA to use constants as PHI arguments. If the constant from s =0 would not be propagated into the PHI, the statement would not be dead and removed, and no new s=0 would have to be inserted in the wrong place. See bug 42906 for another example of exactly the same problem. Allowing constants as PHI arguments is GCC's biggest design f*ck-up. -- http://gcc.gnu.org/bugzilla/show_bug.cgi?id=42839
[Bug rtl-optimization/42839] [4.5 Regression] gcc.target/mips/octeon-bbit-2.c failing for -mabi=64
--- Comment #4 from pinskia at gcc dot gnu dot org 2010-02-12 22:56 --- But that is not the cause of the problem here. The issue comes down to we simplify in 4.5 (subreg (sign_extend (reg N) ) ) into (reg N). fwprop1 is doing that change on the trunk. -- http://gcc.gnu.org/bugzilla/show_bug.cgi?id=42839
[Bug rtl-optimization/42839] [4.5 Regression] gcc.target/mips/octeon-bbit-2.c failing for -mabi=64
--- Comment #5 from steven at gcc dot gnu dot org 2010-02-12 23:08 --- Can you explain how? It's not clear from the code of your comment #2 how a simplification would cause the extra basic block. -- http://gcc.gnu.org/bugzilla/show_bug.cgi?id=42839
[Bug rtl-optimization/42839] [4.5 Regression] gcc.target/mips/octeon-bbit-2.c failing for -mabi=64
--- Comment #6 from pinskia at gcc dot gnu dot org 2010-02-12 23:14 --- (In reply to comment #5) Can you explain how? It's not clear from the code of your comment #2 how a simplification would cause the extra basic block. sorry if I was not clear here but there are two different issues with this testcase. the extra basic block is there for 4.4 still; just it uses the branch likely instruction which is what the testcase is trying to test for. (for those who don't know the MIPS ISA, branch likely says the delay slot is only executed if the branch is taken). The extra simplification is causing the branch likely instruction not to used as there is a way to combine the sign extend with the addition. So there is an extra instruction introduced early on. -- http://gcc.gnu.org/bugzilla/show_bug.cgi?id=42839
[Bug rtl-optimization/42839] [4.5 Regression] gcc.target/mips/octeon-bbit-2.c failing for -mabi=64
--- Comment #7 from pinskia at gcc dot gnu dot org 2010-02-12 23:35 --- (In reply to comment #4) But that is not the cause of the problem here. The issue comes down to we simplify in 4.5 (subreg (sign_extend (reg N) ) ) into (reg N). fwprop1 is doing that change on the trunk. Going back to revision 151021 (one right before the fix for PR 41081), fixes the regression as we no longer froward prop the simplification any more. -- pinskia at gcc dot gnu dot org changed: What|Removed |Added CC||amodra at gmail dot com http://gcc.gnu.org/bugzilla/show_bug.cgi?id=42839
