[Bug rtl-optimization/57300] New: statement in expression miscompiled at -O3 in 32-bit mode

[Bug rtl-optimization/57300] New: statement in expression miscompiled at -O3 in 32-bit mode

[dhazeghi at yahoo dot com]

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

            Bug ID: 57300
           Summary: statement in expression miscompiled at -O3 in 32-bit
                    mode
           Product: gcc
           Version: 4.9.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: rtl-optimization
          Assignee: unassigned at gcc dot gnu.org
          Reporter: dhazeghi at yahoo dot com

The following testcase appears to be miscompiled with gcc 4.8 and trunk on
x86_64-linux-gnu at -O3 optimization level in 32-bit mode.  In 64-bit mode, at
lower optimization, or with gcc 4.7 and earlier it works correctly.

$ gcc-trunk -v
Target: x86_64-unknown-linux-gnu
...
gcc version 4.9.0 20130515 (experimental) [trunk revision 198926] (GCC) 
$ gcc-trunk -O2 -m32 small.c
$ ./a.out 
1
$ gcc-trunk -O3 -m64 small.c
$ ./a.out 
1
$ gcc-4.7 -O3 -m32 small.c
$ ./a.out 
1
$ gcc-trunk -O3 -m32 small.c
$ ./a.out 
0
$

------------------------------

int printf (const char *, ...);
int a, b, e;
long long c;
int d[10];
int
main ()
{
    int *f = &a;
    e = 0;
    for (; e < 10; e++)
        d[e] = 1;
    if (d[0])
        c = (
    {
        int g = *f = (
        {
            b == 0 || 1 % b;
        });
        g;
    }
    );
    printf ("%d\n", a);
    return 0;
}

[Bug rtl-optimization/57300] [4.8/4.9 Regression] statement in expression miscompiled at -O3 in 32-bit mode

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jakub at gcc dot gnu.org
            Version|4.9.0                       |4.8.1
   Target Milestone|---                         |4.8.1
            Summary|statement in expression     |[4.8/4.9 Regression]
                   |miscompiled at -O3 in       |statement in expression
                   |32-bit mode                 |miscompiled at -O3 in
                   |                            |32-bit mode

--- Comment #1 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
/* PR rtl-optimization/57300 */
/* { dg-do run } */
/* { dg-options "-O3" } */
/* { dg-additional-options "-msse2" { target sse2_runtime } } */

extern void abort (void);
int a, b, d[10];
long long c;

int
main ()
{
  int e;
  for (e = 0; e < 10; e++)
    d[e] = 1;
  if (d[0])
    c = a = (b == 0 || 1 % b);
  if (a != 1)
    abort ();
  return 0;
}

(note, -msse needed, without it it doesn't reproduce).  Started with LRA merge,
but looks very much PR57281 related.

[Bug rtl-optimization/57300] [4.8/4.9 Regression] statement in expression miscompiled at -O3 in 32-bit mode

[jakub at gcc dot gnu.org]

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

--- Comment #2 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Created attachment 30130
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=30130&action=edit
gcc49-pr57300.patch

Untested fix.

[Bug rtl-optimization/57300] [4.8/4.9 Regression] statement in expression miscompiled at -O3 in 32-bit mode

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |ebotcazou at gcc dot gnu.org,
                   |                            |krebbel at gcc dot gnu.org,
                   |                            |steven at gcc dot gnu.org

--- Comment #3 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
I'm worried if this isn't too expensive, as there are just 3 dead_or_set_p uses
in define_split patterns in the compiler apparently, one in i386.md (the one
hitting in this and the other PR), guarded with reload_completed, and two for
movdi splitters in s390.md (not guarded by that).  If the s390.md splitters
could be guarded by reload_completed, we could at least avoid the df_analyze in
split1 pass.

[Bug rtl-optimization/57300] [4.8/4.9 Regression] statement in expression miscompiled at -O3 in 32-bit mode

[jakub at gcc dot gnu.org]

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

--- Comment #4 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
The alternative could be, if dead_or_set_p is going to be so rare even in the
near future, to just introduce split_dead_or_set_p wrapper around it, which
would do something like:
  if (df_note == NULL)
    {
      df_note_add_problem ();
      df_analyze ();
    }
  return dead_or_set_p (...);
Not sure if it is safe to run df_analyze when the cfg is gone (split5 pass), if
not, then this wrapper should detect that and just return conservatively false.

[Bug rtl-optimization/57300] [4.8/4.9 Regression] statement in expression miscompiled at -O3 in 32-bit mode

[steven at gcc dot gnu.org]

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

--- Comment #5 from Steven Bosscher <steven at gcc dot gnu.org> ---
(In reply to Jakub Jelinek from comment #4)
> Not sure if it is safe to run df_analyze when the cfg is gone (split5
> pass)

Well, it doesn't crash but it's not correct. Without basic block
pointers on the insns, even the scan problem doesn't run properly
anymore.

One of the target machine reorgs does this, FWIW.

[Bug rtl-optimization/57300] [4.8/4.9 Regression] statement in expression miscompiled at -O3 in 32-bit mode

[jakub at gcc dot gnu.org]

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

--- Comment #6 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Apparently split* passes aren't the only passes that split insns though, so the
patch I've attached is incomplete anyway.
>From quick skimming, it seems split*, dbr, final, combine and pro_and_epilogue
passes split insns right now, and some backends (but not i386/s390 that are
problematic here).  split* passes don't run df_analyze at all, so I think it is
safe to call df_note_add_problem (); df_analyze (); there, final/dbr don't have
cfg around, so it is unsafe, combine already has note problem computed, and
pro_and_epilogue only splits eh_return pattern.
So supposedly
bool
split_dead_or_set_p (rtx insn, const_rtx x)
{
  if (BLOCK_FOR_INSN (insn) == NULL)
    return false; /* If cfg is gone, be conservative.  */
  if (df_note == NULL)
    {
      df_note_add_problem ();
      df_analyze ();
    }
  return dead_or_set_p (insn, x);
}

could work.  Steven, comments?

[Bug rtl-optimization/57300] [4.8/4.9 Regression] statement in expression miscompiled at -O3 in 32-bit mode

[ebotcazou at gcc dot gnu.org]

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

Eric Botcazou <ebotcazou at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2013-05-16
     Ever confirmed|0                           |1

--- Comment #7 from Eric Botcazou <ebotcazou at gcc dot gnu.org> ---
> I'm worried if this isn't too expensive, as there are just 3 dead_or_set_p
> uses in define_split patterns in the compiler apparently, one in i386.md (the
> one hitting in this and the other PR), guarded with reload_completed, and two
> for movdi splitters in s390.md (not guarded by that).  If the s390.md
> splitters could be guarded by reload_completed, we could at least avoid the
> df_analyze in split1 pass.

I agree that calling DF for just 3 cases sounds a bit overkill.  Is the i386.md
splitter really necessary or could we use a peephole2 instead?

[Bug rtl-optimization/57300] [4.8/4.9 Regression] statement in expression miscompiled at -O3 in 32-bit mode

[jakub at gcc dot gnu.org]

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

--- Comment #8 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Created attachment 30131
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=30131&action=edit
/tmp/gcc49-pr57300.patch

The split_dead_or_set_p variant patch.

[Bug rtl-optimization/57300] [4.8/4.9 Regression] statement in expression miscompiled at -O3 in 32-bit mode

[steven at gcc dot gnu.org]

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

--- Comment #9 from Steven Bosscher <steven at gcc dot gnu.org> ---
(In reply to Jakub Jelinek from comment #6)
> So supposedly
> bool
> split_dead_or_set_p (rtx insn, const_rtx x)
> {
>   if (BLOCK_FOR_INSN (insn) == NULL)
>     return false; /* If cfg is gone, be conservative.  */
>   if (df_note == NULL)
>     {
>       df_note_add_problem ();
>       df_analyze ();
>     }
>   return dead_or_set_p (insn, x);
> }
> 
> could work.  Steven, comments?

I don't think this will work. The fact that df_note != NULL does not
mean the notes are up-to-date.

It seems to me that after freeing the CFG, no pass should rely on the
REG_DEAD and REG_UNUSED notes if Eric's comment in PR57281 is correct:
That each pass is responsible for re-computing the notes if it needs
them. But I would much rather have the passes at least discard the
notes if the pass can't update them.  After all, it's not just this
dead_or_set_p function that relies on these notes, but also single_set
and a bunch of other functions.

[Bug rtl-optimization/57300] [4.8/4.9 Regression] statement in expression miscompiled at -O3 in 32-bit mode

[jakub at gcc dot gnu.org]

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

--- Comment #10 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Re: Eric's question, not easily, because if we implement that define_split as
define_peephole2, then it won't trigger at all, because the next define_split
";; Extend to memory case when source register does not die.", will then split
the insn already during split2 pass rather than waiting for peephole2 pass.

So we'd need to protect that second splitter with peephole2_completed predicate
which we don't have yet, or so.  Is that the way to go?  If so, I can try a
patch.  Note, s390 will still be broken, but we don't have testcases for it
right now.

Re: Steven, we need some solution for 4.8.1 too.  While it is true that
generally df_note doesn't mean the problem is up2date, doesn't df_finish_pass
always kill that problem and thus at least the current pass should have called
df_add_note_problem?  Given the passes that split insns right now (see previous
comments), having df_note non-NULL means either it is a combine pass where it
is fine, or split* pass in which split_dead_or_set_p has been called already.

[Bug rtl-optimization/57300] [4.8/4.9 Regression] statement in expression miscompiled at -O3 in 32-bit mode

[jakub at gcc dot gnu.org]

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

--- Comment #11 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Re: s390, the condition includes ACCESS_REG_P test, which is for a0/a1 hard
reg, so I think just adding reload_completed test to the splitters wouldn't be
a problem, and if we introduce peephole2_completed, we might deal with it the
same way (one splitter changed into define_peephole2, the other guarded with
peephole2_completed).

[Bug rtl-optimization/57300] [4.8/4.9 Regression] statement in expression miscompiled at -O3 in 32-bit mode

[rguenth at gcc dot gnu.org]

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

--- Comment #13 from Richard Biener <rguenth at gcc dot gnu.org> ---
(In reply to Steven Bosscher from comment #9)
> (In reply to Jakub Jelinek from comment #6)
> > So supposedly
> > bool
> > split_dead_or_set_p (rtx insn, const_rtx x)
> > {
> >   if (BLOCK_FOR_INSN (insn) == NULL)
> >     return false; /* If cfg is gone, be conservative.  */
> >   if (df_note == NULL)
> >     {
> >       df_note_add_problem ();
> >       df_analyze ();
> >     }
> >   return dead_or_set_p (insn, x);
> > }
> > 
> > could work.  Steven, comments?
> 
> I don't think this will work. The fact that df_note != NULL does not
> mean the notes are up-to-date.
> 
> It seems to me that after freeing the CFG, no pass should rely on the
> REG_DEAD and REG_UNUSED notes if Eric's comment in PR57281 is correct:
> That each pass is responsible for re-computing the notes if it needs
> them. But I would much rather have the passes at least discard the
> notes if the pass can't update them.  After all, it's not just this
> dead_or_set_p function that relies on these notes, but also single_set
> and a bunch of other functions.

I agree that possibly bogus IL is a smoking gun that waits for this kind
of bugs to appear.  If we do not want to pay the price of removing
notes can we at least have a flag that tells whether the NOTE problem
is up-to-date (and add verification that it indeed is - at least has
no bogus notes - when that flag is set)?  We could introduce
a new PROP_rtl_notes for that.

As for the case in question in PR57281 - it shouldn't be hard for
postreload to remove REG_DEAD notes from all uses when it propagates
equivalencies, no?

Btw, as alternative to computing the NOTE problem at the start of split
we can also remove all notes which should be cheaper.

[Bug rtl-optimization/57300] [4.8/4.9 Regression] statement in expression miscompiled at -O3 in 32-bit mode

[ebotcazou at gcc dot gnu.org]

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

--- Comment #14 from Eric Botcazou <ebotcazou at gcc dot gnu.org> ---
> I agree that possibly bogus IL is a smoking gun that waits for this kind
> of bugs to appear.  If we do not want to pay the price of removing
> notes can we at least have a flag that tells whether the NOTE problem
> is up-to-date (and add verification that it indeed is - at least has
> no bogus notes - when that flag is set)?  We could introduce
> a new PROP_rtl_notes for that.

We cannot be sure that the NOTE problem is up-to-date since DF doesn't update
them on the fly; it's correct only right after calling df_analyze.

> As for the case in question in PR57281 - it shouldn't be hard for
> postreload to remove REG_DEAD notes from all uses when it propagates
> equivalencies, no?

All the dataflow stuff is now handled by DF and I don't think that we should go
backwards by asking again individual passes to fiddle with it.

> Btw, as alternative to computing the NOTE problem at the start of split
> we can also remove all notes which should be cheaper.

Yes, that's another possibility.

[Bug rtl-optimization/57300] [4.8/4.9 Regression] statement in expression miscompiled at -O3 in 32-bit mode

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

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

--- Comment #15 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Author: jakub
Date: Fri May 17 15:22:24 2013
New Revision: 199018

URL: http://gcc.gnu.org/viewcvs?rev=199018&root=gcc&view=rev
Log:
    PR rtl-optimization/57281
    PR rtl-optimization/57300
    * config/i386/i386.md (extendsidi2_1 dead reg splitter): Remove.
    (extendsidi2_1 peephole2s): Add instead 2 new peephole2s, that undo
    what the other splitter did if the registers are dead.

    * gcc.dg/pr57300.c: New test.
    * gcc.c-torture/execute/pr57281.c: New test.

Added:
    trunk/gcc/testsuite/gcc.c-torture/execute/pr57281.c
    trunk/gcc/testsuite/gcc.dg/pr57300.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/config/i386/i386.md
    trunk/gcc/testsuite/ChangeLog

Author: jakub
Date: Fri May 17 15:32:50 2013
New Revision: 199021

URL: http://gcc.gnu.org/viewcvs?rev=199021&root=gcc&view=rev
Log:
    PR rtl-optimization/57281
    PR rtl-optimization/57300
    * config/i386/i386.md (extendsidi2_1 dead reg splitter): Remove.
    (extendsidi2_1 peephole2s): Add instead 2 new peephole2s, that undo
    what the other splitter did if the registers are dead.

    * gcc.dg/pr57300.c: New test.
    * gcc.c-torture/execute/pr57281.c: New test.

Added:
    branches/gcc-4_8-branch/gcc/testsuite/gcc.c-torture/execute/pr57281.c
    branches/gcc-4_8-branch/gcc/testsuite/gcc.dg/pr57300.c
Modified:
    branches/gcc-4_8-branch/gcc/ChangeLog
    branches/gcc-4_8-branch/gcc/config/i386/i386.md
    branches/gcc-4_8-branch/gcc/testsuite/ChangeLog