[Bug tree-optimization/33434] New: [4.3 Regression] -fipa-cp miscompilation

[Bug tree-optimization/33434] New: [4.3 Regression] -fipa-cp miscompilation

[belyshev at depni dot sinp dot msu dot ru]

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;
}


-- 
           Summary: [4.3 Regression] -fipa-cp miscompilation
           Product: gcc
           Version: 4.3.0
            Status: UNCONFIRMED
          Keywords: wrong-code
          Severity: normal
          Priority: P3
         Component: tree-optimization
        AssignedTo: unassigned at gcc dot gnu dot org
        ReportedBy: belyshev at depni dot sinp dot msu dot ru


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=33434

[Bug tree-optimization/33434] [4.3 Regression] -fipa-cp miscompilation

[razya at il dot ibm dot com]

------- Comment #1 from razya at il dot ibm dot com  2007-09-16 10:38 -------
(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


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=33434

[Bug tree-optimization/33434] [4.3 Regression] -fipa-cp miscompilation

[pinskia at gcc dot gnu dot org]

-- 

pinskia at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |4.3.0


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=33434

[Bug tree-optimization/33434] [4.3 Regression] -fipa-cp miscompilation

[mmitchel at gcc dot gnu dot org]

-- 

mmitchel at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P3                          |P1


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=33434

[Bug tree-optimization/33434] [4.3 Regression] -fipa-cp miscompilation

[jakub at gcc dot gnu dot org]

------- Comment #2 from jakub at gcc dot gnu dot org  2007-10-12 13:38 -------
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.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=33434

[Bug tree-optimization/33434] [4.3 Regression] -fipa-cp miscompilation

[janis at gcc dot gnu dot org]

------- Comment #3 from janis at gcc dot gnu dot org  2007-10-19 19:52 -------
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)


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=33434

[Bug tree-optimization/33434] [4.3 Regression] -fipa-cp miscompilation

[pinskia at gcc dot gnu dot org]

------- Comment #4 from pinskia at gcc dot gnu dot org  2007-10-20 00:05 -------
(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;
}


-- 

pinskia at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |janis at gcc dot gnu dot org
             Status|UNCONFIRMED                 |NEW
     Ever Confirmed|0                           |1
   Last reconfirmed|0000-00-00 00:00:00         |2007-10-20 00:05:28
               date|                            |


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=33434

[Bug tree-optimization/33434] [4.3 Regression] -fipa-cp miscompilation

[pinskia at gcc dot gnu dot org]

------- Comment #5 from pinskia at gcc dot gnu dot org  2007-10-20 00:18 -------
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;
}


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=33434

[Bug tree-optimization/33434] [4.3 Regression] -fipa-cp miscompilation

[pinskia at gcc dot gnu dot org]

------- Comment #6 from pinskia at gcc dot gnu dot org  2007-10-28 00:30 -------
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.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=33434

[Bug tree-optimization/33434] [4.3 Regression] -fipa-cp miscompilation

[pinskia at gcc dot gnu dot org]

------- Comment #7 from pinskia at gcc dot gnu dot org  2007-10-28 00:40 -------
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. 


-- 

pinskia at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |pinskia at gcc dot gnu dot
                   |                            |org, hubicka at gcc dot gnu
                   |                            |dot org


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=33434

[Bug tree-optimization/33434] [4.3 Regression] inlining miscompilation

[rguenth at gcc dot gnu dot org]

------- Comment #8 from rguenth at gcc dot gnu dot org  2007-10-28 18:57 -------
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).


-- 

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=33434

[Bug tree-optimization/33434] [4.3 Regression] inlining miscompilation

[hubicka at gcc dot gnu dot org]

------- Comment #9 from hubicka at gcc dot gnu dot org  2007-10-28 19:17 -------
Mine


-- 

hubicka at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         AssignedTo|unassigned at gcc dot gnu   |hubicka at gcc dot gnu dot
                   |dot org                     |org
             Status|NEW                         |ASSIGNED
   Last reconfirmed|2007-10-20 00:05:28         |2007-10-28 19:17:48
               date|                            |


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=33434

[Bug tree-optimization/33434] [4.3 Regression] inlining miscompilation

[razya at il dot ibm dot com]

------- 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?

Razya 


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=33434

[Bug tree-optimization/33434] [4.3 Regression] inlining miscompilation

[rguenther at suse dot de]

------- 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.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=33434

[Bug tree-optimization/33434] [4.3 Regression] inlining miscompilation

[razya at il dot ibm dot com]

------- Comment #12 from razya at il dot ibm dot com  2007-10-29 13:00 -------
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.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=33434

[Bug tree-optimization/33434] [4.3 Regression] inlining miscompilation

[rguenther at suse dot de]

------- 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.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=33434

[Bug tree-optimization/33434] [4.3 Regression] inlining miscompilation

[razya at il dot ibm dot com]

------- Comment #14 from razya at il dot ibm dot com  2007-10-30 09:14 -------
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.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=33434

[Bug tree-optimization/33434] [4.3 Regression] inlining miscompilation

[hubicka at gcc dot gnu dot org]

------- Comment #15 from hubicka at gcc dot gnu dot org  2007-11-08 12:42 -------
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.  */


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=33434

[Bug tree-optimization/33434] [4.3 Regression] inlining miscompilation

[jakub at gcc dot gnu dot org]

------- Comment #16 from jakub at gcc dot gnu dot org  2007-11-16 20:29 -------
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);
}


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=33434

[Bug tree-optimization/33434] [4.3 Regression] inlining miscompilation

[pinskia at gcc dot gnu dot org]

------- Comment #17 from pinskia at gcc dot gnu dot org  2007-11-23 05:33 -------
What about using the copied default SSA_NAME for the function arguments?  That
seems better and you don't need to rename the names.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=33434

[Bug tree-optimization/33434] [4.3 Regression] inlining miscompilation

[pinskia at gcc dot gnu dot org]

------- Comment #18 from pinskia at gcc dot gnu dot org  2007-11-23 05:37 -------
(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.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=33434

[Bug tree-optimization/33434] [4.3 Regression] inlining miscompilation

[jakub at gcc dot gnu dot org]

------- Comment #19 from jakub at gcc dot gnu dot org  2007-11-29 18:39 -------
Have a modified patch which cures this, testing...


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=33434

[Bug tree-optimization/33434] [4.3 Regression] inlining miscompilation

[jakub at gcc dot gnu dot org]

------- Comment #20 from jakub at gcc dot gnu dot org  2007-11-29 21:57 -------
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


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=33434

[Bug tree-optimization/33434] [4.3 Regression] inlining miscompilation

[jakub at gcc dot gnu dot org]

------- Comment #21 from jakub at gcc dot gnu dot org  2007-11-29 21:58 -------
Fixed.


-- 

jakub at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|                            |FIXED


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=33434