This is GCC Bugzilla
This is GCC Bugzilla Version 2.20+
View Bug Activity | Format For Printing | Clone This Bug
Found this while trying to bootstrap with BOOT_CFLAGS="-O3 -fipa-cp" and it miscompiled libcpp/macro.c at the stage2. Appeared somewhere in between r122374 and r124593: /* { dg-do run } */ /* { dg-options "-O3 -fipa-cp" } */ int k; void f1 (int a, int b) { if (a) while (b --) k = 1; else if (b != 1) __builtin_abort (); } int main (void) { f1 (1, 1); if (k != 1) __builtin_abort (); return 0; }
(In reply to comment #0) > Found this while trying to bootstrap with BOOT_CFLAGS="-O3 -fipa-cp" and it > miscompiled libcpp/macro.c at the stage2. Appeared somewhere in between > r122374 and r124593: > /* { dg-do run } */ > /* { dg-options "-O3 -fipa-cp" } */ > int k; > void f1 (int a, int b) > { > if (a) > while (b --) > k = 1; > else > if (b != 1) > __builtin_abort (); > } > int main (void) > { > f1 (1, 1); > if (k != 1) > __builtin_abort (); > return 0; > } Looking into it. Razya
Doesn't seem to look like a problem in ipa-cp to me, more like an inlining bug. ipa-cp creates: T.1 (a, b) { _Bool D.1565; <bb 2>: if (1) goto <bb 3>; else goto <bb 6>; <bb 3>: # b_3 = PHI <1(2)> goto <bb 5>; <bb 4>: k ={v} 1; <bb 5>: # b_2 = PHI <b_3(3), b_1(4)> b_1 = b_2 + -1; if (b_2 != 0) goto <bb 4>; else goto <bb 8>; <bb 6>: if (0) goto <bb 7>; else goto <bb 8>; <bb 7>: __builtin_abort (); <bb 8>: return; } and even the first dump during apply_inline looks correct, inside of main we have: ... # BLOCK 4 freq:4999 # b_6 = PHI <1(3)> goto <bb 6>; # BLOCK 5 freq:10000 k ={v} 1; # BLOCK 6 freq:10000 # b_5 = PHI <b_6(4), b_4(5)> b_4 = b_5 + -1; if (b_5 != 0) goto <bb 5>; else goto <bb 9>; But during inlining both a and b are mark_sym_for_renaming and when update_ssa is called, rewrite_update_stmt -> maybe_replace_use changes that if (b_5 != 0) into incorrect: if (b_4 != 0) so k is never set to 1.
A regression hunt on powerpc-linux identified this patch: http://gcc.gnu.org/viewcvs?view=rev&rev=124353 r124353 | pinskia | 2007-05-02 17:47:06 +0000 (Wed, 02 May 2007)
(In reply to comment #3) > A regression hunt on powerpc-linux identified this patch: HEHEHEHEHEHEHE. Seriously this is funny. Anyways try changing the code to be (which will not invoke my removal of the "optimization"): /* { dg-do run } */ /* { dg-options "-O3 -fipa-cp" } */ int k; void f1 (int a, int b) { if (a) { int c = b--; while (c) k = 1; } else if (b != 1) __builtin_abort (); } int main (void) { f1 (1, 1); if (k != 1) __builtin_abort (); return 0; }
Try this one: /* { dg-do run } */ /* { dg-options "-O3 -fipa-cp" } */ int k; void f1 (int a, int b) { if (a) { int c; goto d; do { k = 1; d: c = b--; }while (c); } else if (b != 1) __builtin_abort (); } int main (void) { f1 (1, 1); if (k != 1) __builtin_abort (); return 0; }
Hmm, I have a question about IPA CP, should it call cfgcleanup also? It does not fix the problem here but it seems like a good idea. I can test a patch which adds the cfgcleanup if it is a good idea.
Here is a testcase without using IPA CP: /* { dg-do run } */ /* { dg-options "-O3" } */ int k; void f1 (int a, int b) { a = 1; b = 1; if (a) { int c; goto d; do { k = 1; d: c = b--; }while (c); } else if (b != 1) __builtin_abort (); } int main (void) { f1 (1, 1); if (k != 1) __builtin_abort (); return 0; } ----- CUT ----- Jan, Can you please look into this, this is an inlining issue where we add a statement for the assignment of a/b and mark that symbol for renaming and it causes the renaming to mess up as SSA rename can't handle overlapping names.
mark_sym_for_renaming really needs some checking code for this. Or mark_sym_for_renaming needs to properly go out-of-ssa for the symbol (in the case of inlining separately so for the caller and the callee).
Mine
(In reply to comment #6) > Hmm, I have a question about IPA CP, should it call cfgcleanup also? It does > not fix the problem here but it seems like a good idea. I can test a patch > which adds the cfgcleanup if it is a good idea. Hi Andrew IPA CP iterates the whole callgraph, so do you mean cfgcleanup for each function? Razya
Subject: Re: [4.3 Regression] inlining miscompilation On Mon, 29 Oct 2007, razya at il dot ibm dot com wrote: > ------- Comment #10 from razya at il dot ibm dot com 2007-10-29 12:08 ------- > (In reply to comment #6) > > Hmm, I have a question about IPA CP, should it call cfgcleanup also? It does > > not fix the problem here but it seems like a good idea. I can test a patch > > which adds the cfgcleanup if it is a good idea. > > Hi Andrew > IPA CP iterates the whole callgraph, so do you mean cfgcleanup for each > function? Only for the clones it propagated constants into. Richard.
Subject: Re: [4.3 Regression] inlining miscompilation "rguenther at suse dot de" <gcc-bugzilla@gcc.gnu.org> wrote on 29/10/2007 14:14:45: > > > ------- Comment #11 from rguenther at suse dot de 2007-10-29 12:14 ------- > Subject: Re: [4.3 Regression] inlining > miscompilation > > On Mon, 29 Oct 2007, razya at il dot ibm dot com wrote: > > > ------- Comment #10 from razya at il dot ibm dot com 2007-10-29 > 12:08 ------- > > (In reply to comment #6) > > > Hmm, I have a question about IPA CP, should it call cfgcleanup > also? It does > > > not fix the problem here but it seems like a good idea. I can > test a patch > > > which adds the cfgcleanup if it is a good idea. > > > > Hi Andrew > > IPA CP iterates the whole callgraph, so do you mean cfgcleanup for each > > function? > > Only for the clones it propagated constants into. > > Richard. > IPA CP basically replaces the uses of the (always consatnt)parameter with the constant. This can be further folded by the ssa-cp pass on the cloned method. So I'm not sure how necessary it is to have a control flow cleanup at this stage, but maybe I'm wrong... Razya > > -- > > > http://gcc.gnu.org/bugzilla/show_bug.cgi?id=33434 > > ------- You are receiving this mail because: ------- > You are on the CC list for the bug, or are watching someone who is.
Subject: Re: [4.3 Regression] inlining miscompilation On Mon, 29 Oct 2007, razya at il dot ibm dot com wrote: > > ------- Comment #11 from rguenther at suse dot de 2007-10-29 12:14 > ------- > > Subject: Re: [4.3 Regression] inlining > > miscompilation > > > > On Mon, 29 Oct 2007, razya at il dot ibm dot com wrote: > > > > > ------- Comment #10 from razya at il dot ibm dot com 2007-10-29 > > 12:08 ------- > > > (In reply to comment #6) > > > > Hmm, I have a question about IPA CP, should it call cfgcleanup > > also? It does > > > > not fix the problem here but it seems like a good idea. I can > > test a patch > > > > which adds the cfgcleanup if it is a good idea. > > > > > > Hi Andrew > > > IPA CP iterates the whole callgraph, so do you mean cfgcleanup for > each > > > function? > > > > Only for the clones it propagated constants into. > > > > Richard. > > > > IPA CP basically replaces the uses of the (always consatnt)parameter > with the constant. > This can be further folded by the ssa-cp pass on the cloned method. > So I'm not sure how necessary it is to have a control flow cleanup > at this stage, but maybe I'm wrong... Just because propagating the constants might result in if (0) ... which cfg_cleanup is able to remove (or if (1)). Richard.
Subject: Re: [4.3 Regression] inlining miscompilation "rguenther at suse dot de" <gcc-bugzilla@gcc.gnu.org> wrote on 29/10/2007 15:12:36: > > > ------- Comment #13 from rguenther at suse dot de 2007-10-29 13:12 ------- > Subject: Re: [4.3 Regression] inlining > miscompilation > > On Mon, 29 Oct 2007, razya at il dot ibm dot com wrote: > > > > ------- Comment #11 from rguenther at suse dot de 2007-10-29 12:14 > > ------- > > > Subject: Re: [4.3 Regression] inlining > > > miscompilation > > > > > > On Mon, 29 Oct 2007, razya at il dot ibm dot com wrote: > > > > > > > ------- Comment #10 from razya at il dot ibm dot com 2007-10-29 > > > 12:08 ------- > > > > (In reply to comment #6) > > > > > Hmm, I have a question about IPA CP, should it call cfgcleanup > > > also? It does > > > > > not fix the problem here but it seems like a good idea. I can > > > test a patch > > > > > which adds the cfgcleanup if it is a good idea. > > > > > > > > Hi Andrew > > > > IPA CP iterates the whole callgraph, so do you mean cfgcleanup for > > each > > > > function? > > > > > > Only for the clones it propagated constants into. > > > > > > Richard. > > > > > > > IPA CP basically replaces the uses of the (always consatnt)parameter > > with the constant. > > This can be further folded by the ssa-cp pass on the cloned method. > > So I'm not sure how necessary it is to have a control flow cleanup > > at this stage, but maybe I'm wrong... > > Just because propagating the constants might result in > > if (0) > ... > > which cfg_cleanup is able to remove (or if (1)). > > Richard. > > O.k, sure. Thanks. > -- > > > http://gcc.gnu.org/bugzilla/show_bug.cgi?id=33434 > > ------- You are receiving this mail because: ------- > You are on the CC list for the bug, or are watching someone who is.
The problem here is that code assumes that variable without DEFAULT_DEF is not in SSA. Arguments that are unused also do have DEFAULT_DEF=NULL, this patch should fix it. I am testing it now. Honza Index: tree-inline.c =================================================================== *** tree-inline.c (revision 129992) --- tree-inline.c (working copy) *************** setup_one_parameter (copy_body_data *id, *** 1420,1425 **** --- 1420,1434 ---- && !useless_type_conversion_p (TREE_TYPE (p), TREE_TYPE (value))) rhs = fold_build1 (NOP_EXPR, TREE_TYPE (p), value); + /* If the value of argument is never used, don't care about initializing it. + This is needed for correctness since we otherwise confuse it with non-SSA + variable later and end up renaming otherwise. */ + if (gimple_in_ssa_p (cfun) && is_gimple_reg (p) && !def) + { + gcc_assert (!TREE_SIDE_EFFECTS (value)); + return; + } + /* If the parameter is never assigned to, has no SSA_NAMEs created, we may not need to create a new variable here at all. Instead, we may be able to just use the argument value. */
Tried to bootstrap that, and while just for make check it basically just needed gcc_assert (!value || !TREE_SIDE_EFFECTS (value)); because e.g. some Fortran testcases have value == NULL, bootstrap fails, e.g. on i386.c. Simplified testcase: void * baz (void); static void * bar (void *x) { x = baz (); return x; } void * foo (void *x) { return bar (x); }
What about using the copied default SSA_NAME for the function arguments? That seems better and you don't need to rename the names.
(In reply to comment #17) > What about using the copied default SSA_NAME for the function arguments? That > seems better and you don't need to rename the names. Actually ignore this, I have not read the inliner code until now and see it already does that.
Have a modified patch which cures this, testing...
Subject: Bug 33434 Author: jakub Date: Thu Nov 29 21:57:38 2007 New Revision: 130521 URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=130521 Log: PR tree-optimization/33434 * tree-inline.c (setup_one_parameter): If the value passed to a parameter is never used, don't set it up. * gcc.dg/pr33434-1.c: New test. * gcc.dg/pr33434-2.c: New test. * gcc.dg/pr33434-3.c: New test. * gcc.dg/pr33434-4.c: New test. Added: trunk/gcc/testsuite/gcc.dg/pr33434-1.c trunk/gcc/testsuite/gcc.dg/pr33434-2.c trunk/gcc/testsuite/gcc.dg/pr33434-3.c trunk/gcc/testsuite/gcc.dg/pr33434-4.c Modified: trunk/gcc/ChangeLog trunk/gcc/testsuite/ChangeLog trunk/gcc/tree-inline.c
Fixed.