[Bug c/12799] New: cmov is confused by a intervening inc that bashes flags.

[Bug c/12799] New: cmov is confused by a intervening inc that bashes flags.

[pratap at vmware dot com]

PLEASE REPLY TO gcc-bugzilla@gcc.gnu.org ONLY, *NOT* gcc-bugs@gcc.gnu.org.

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

           Summary: cmov is confused by a intervening inc that bashes flags.
           Product: gcc
           Version: 3.3
            Status: UNCONFIRMED
          Severity: critical
          Priority: P1
         Component: c
        AssignedTo: unassigned at gcc dot gnu dot org
        ReportedBy: pratap at vmware dot com
                CC: gcc-bugs at gcc dot gnu dot org

The following program returns 1 (denotes failure), instead of 0 (which
denotes success) when compiled with the following compiler flags,
using gcc 3.0.3, but passes with the same flags on gcc 3.3. 

I'm still filing this as a bug on gcc 3.3 since all my bugzilla searches 
did not convince me that the bug has indeed been fixed in 3.3. 

Here are the flags: 
    -O2 -g3 -march=i686 -fno-strict-aliasing  -pipe
    -mpreferred-stack-boundary=2 -fno-strict-aliasing 
    -march=i686 -mcpu=i686 -malign-jumps=2 -malign-functions=2
    -malign-loops=2 -mregparm=3 -Wmissing-prototypes'

________________________________________________________________________

int loo = 1;

__inline__ char InlineFunc(void);
int FooBar(void);

__inline__ char InlineFunc(void)
{
   return __builtin_expect(!!(loo == 1), 1);
}

int FooBar(void)
{
   int i;
   int var1 = InlineFunc() ? 2046 : 1023;
   int var2 = InlineFunc() ? 512 : 1024;

   for (i = 0; i < var1; i++) {};
   if (InlineFunc() && var2 != 512) {
      return 1;
   }
   return 0;
}

int main() {
   return FooBar();
}

________________________________________________________________________


Here is the assembly generated by the 3.0.3 compiler: 

       <stuff snipped> 

        movl    $1023, %eax
        movl    $2046, %ecx
        movl    %esp, %ebp
        pushl   %ebx
        cmpl    $1, %edx
        movl    $512, %ebx
        cmovne  %eax, %ecx
        incl    %eax                    <<<< bashes flags
        cmovne  %eax, %ebx              <<<< needs those old flags!
        
       <stuff snipped>

It seems that an "add %eax = %eax + 1" in the intermediate graph
was replaced incorrectly with an "inc %eax".

When I try this with the 3.3 compiler, I see: 

        sete    %al
        testb   $1, %al
        movl    $1023, %eax
        cmove   %eax, %ecx
        cmpl    $1, %edx
        sete    %al
        testb   $1, %al
        movl    $1024, %eax   <<< this is fine, it doenst bash flags. 
        cmove   %eax, %ebx
        testl   %ecx, %ecx

It is comforting to know that gcc 3.3 does not exhibit problem. What I
want to know is if 3.3 really fixes the problem, or is it some accident
of compiler phase ordering. I did a deep search of the gcc.gnu.org
bugzilla database but did not find any reported bugs that seemed like
this one. If I have missed it, could you point me to a bug report, bug
id? 

We would like to move to 3.3 with the confidence that the bug has been
fixed, or else we may be forced to turn off cmov synthesis, which will
be unfortunate since it is a good performance win.

Please advice.

[Bug optimization/12799] cmov is confused by a intervening inc that bashes flags.

[ebotcazou at gcc dot gnu dot org]

PLEASE REPLY TO gcc-bugzilla@gcc.gnu.org ONLY, *NOT* gcc-bugs@gcc.gnu.org.

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


ebotcazou at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
          Component|c                           |optimization
     Ever Confirmed|                            |1
  GCC build triplet|                            |i686-pc-linux-gnu
   GCC host triplet|                            |i686-pc-linux-gnu
 GCC target triplet|                            |i686-pc-linux-gnu
           Priority|P1                          |P2
   Last reconfirmed|0000-00-00 00:00:00         |2003-10-28 08:01:53
               date|                            |
            Summary|cmov is confused by a       |cmov is confused by a
                   |intervening inc that bashes |intervening inc that bashes
                   |flags.                      |flags.
            Version|3.3                         |3.0.4


------- Additional Comments From ebotcazou at gcc dot gnu dot org  2003-10-28 08:01 -------
Confirmed with GCC 3.0.4 but fixed in GCC 3.1.1 and later.

[Bug optimization/12799] cmov is confused by a intervening inc that bashes flags.

[ebotcazou at gcc dot gnu dot org]

PLEASE REPLY TO gcc-bugzilla@gcc.gnu.org ONLY, *NOT* gcc-bugs@gcc.gnu.org.

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



------- Additional Comments From ebotcazou at gcc dot gnu dot org  2003-10-28 08:06 -------
I won't try to find the particular patch that fixed the problem. I can only say
that GCC 3.1.1, 3.2, 3.2.1, 3.2.2, 3.2.3, 3.3, 3.3.1, 3.3.2 and 3.3.3pre produce
exactly the same code at the specified point.

[Bug optimization/12799] cmov is confused by a intervening inc that bashes flags.

[pratap at vmware dot com]

PLEASE REPLY TO gcc-bugzilla@gcc.gnu.org ONLY, *NOT* gcc-bugs@gcc.gnu.org.

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


pratap at vmware dot com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P2                          |P3


------- Additional Comments From pratap at vmware dot com  2003-10-29 17:42 -------
Eric: 

Thanks for validating that the assembly code produced by all these
versions: CC 3.1.1, 3.2, 3.2.1, 3.2.2, 3.2.3, 3.3, 3.3.1, 3.3.2 and 3.3.3pre
are correct. I too had verified that 3.3. produces the correct code 
sequence. 

This problem warrants slightly deeper probing. Which phase of the compiler
had the problem in it? I can see the following possibilities:
    - a local peep hole optimizer that did not do proper local data 
      flow analysis
    - a phase that replaces large instructions with smaller ones may have
      decided that inc was a better deal than an add, w/o realising the 
      data flow implications
    - more scarily, this could be a bug in the local/global data flow 
      analysis module. 
    - others? 

Given this wide spectra of possible culprits, and the general attractiveness 
for folks to want to use cmov'es, I would prefer if GCC could give a more
assurance that the bug has infact been fixed either by pointing out a 
specific patch, or at least explaining by way of reasoning about the current
source base. 

The product that we at VMWare release compiled using GCC goes out to several
million users, so if there is a bug, it is likely to get exposed soon.

[Bug optimization/12799] cmov is confused by a intervening inc that bashes flags.

[ebotcazou at gcc dot gnu dot org]

PLEASE REPLY TO gcc-bugzilla@gcc.gnu.org ONLY, *NOT* gcc-bugs@gcc.gnu.org.

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


ebotcazou at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         AssignedTo|unassigned at gcc dot gnu   |ebotcazou at gcc dot gnu dot
                   |dot org                     |org
             Status|NEW                         |ASSIGNED


------- Additional Comments From ebotcazou at gcc dot gnu dot org  2003-10-29 18:54 -------
I mentioned the 9 versions (coming from two different 3.x series) as an argument
for the robustness of the fix. It is of course not a definitive argument.

The 3.0.x series is not supported any longer, so I should probably close the bug
report at this point. However, I'll quickly take a look tomorrow.

[Bug optimization/12799] cmov is confused by a intervening inc that bashes flags.

[pratap at vmware dot com]

PLEASE REPLY TO gcc-bugzilla@gcc.gnu.org ONLY, *NOT* gcc-bugs@gcc.gnu.org.

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



------- Additional Comments From pratap at vmware dot com  2003-10-29 19:22 -------
I realised that 3.0.3 is not supported when I filed this bug. This is one of 
the reasons that I did not file it as a bug in 3.0.3, but instead as a bug
in 3.3. I suppose I went with the assumption that 3.3 is guilty until proven
innocent (and I'm still in this mode of thinking). 

Thanks very very much for offering to take a deeper look. It will reassure me 
a lot. 

Please note: we _are_ moving away from 3.0.3 no matter what the outcome of this 
discussion. It is only a matter of whether we can trust the cmov synthesis
in the 3.3 compiler or should we disable it (which will be an unfortunate 
thing due to loss of performance).

[Bug optimization/12799] incl does not mark it self as clobbers flags

[pinskia at gcc dot gnu dot org]

PLEASE REPLY TO gcc-bugzilla@gcc.gnu.org ONLY, *NOT* gcc-bugs@gcc.gnu.org.

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


pinskia at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Severity|critical                    |normal
           Keywords|                            |wrong-code
            Summary|cmov is confused by a       |incl does not mark it self
                   |intervening inc that bashes |as clobbers flags
                   |flags.                      |

[Bug optimization/12799] [3.4 regression] faulty mov->add change clobbers the CC register

[ebotcazou at gcc dot gnu dot org]

PLEASE REPLY TO gcc-bugzilla@gcc.gnu.org ONLY, *NOT* gcc-bugs@gcc.gnu.org.

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


ebotcazou at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Severity|normal                      |critical
           Priority|P3                          |P2
            Summary|incl does not mark it self  |[3.4 regression] faulty mov-
                   |as clobbers flags           |>add change clobbers the CC
                   |                            |register
   Target Milestone|---                         |3.4


------- Additional Comments From ebotcazou at gcc dot gnu dot org  2003-10-30 12:47 -------
Well, it turns out that we should probably buy you some beers for your perseverance!

The culprit is the "post-reload" pass which is run right after (global) register
allocation, more precisely its "reload CSE" sub-pass and, even more precisely,
the reload_cse_move2add pass (see reload1.c:9050 and below in the 3.3.x tree).
What it does matches exactly your item #2 ("a phase that replaces large
instructions with...") except that it's a "mov $1024, %eax" -> "incl %eax"
replacement.

Now a good news and a bad news. The good news: the bug can't occur in the 3.3.x
series because reload_cse_move2add is effectively disabled. It was not updated
when a far-reaching change was made; as a consequence, the transformation that
triggered the bug in the 3.0.x series is always rejected (the calls to
validate_change() line 9173 and 9225 always fail).

The bad news: reload_cse_move2add was updated on mainline (which will become GCC
3.4) by

   http://gcc.gnu.org/ml/gcc-patches/2003-04/msg00595.html

so I think the latent bug is now exposed again there.

I'll submit a patch for mainline.

[Bug optimization/12799] [3.4 regression] faulty mov->add change clobbers the CC register

[pratap at vmware dot com]

PLEASE REPLY TO gcc-bugzilla@gcc.gnu.org ONLY, *NOT* gcc-bugs@gcc.gnu.org.

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



------- Additional Comments From pratap at vmware dot com  2003-10-30 13:00 -------
Hmmm ... Pratap like beers ... 

Kidding aside, thanks a bunch for the deep analysis. Your response is perfectly
satisfactory. 

May I make one more suggestion: Before this PR is closed, could the test case
that I submitted be added to the standard regression test suite? The way I have
written the test, it should integrate well into a suite of executable tests, or 
a suite where you compare the generated assembly. 

Thanks, again. We will use 3.3.x with a lot more confidence now.

[Bug optimization/12799] [3.4 regression] faulty mov->add change clobbers the CC register

[ebotcazou at gcc dot gnu dot org]

PLEASE REPLY TO gcc-bugzilla@gcc.gnu.org ONLY, *NOT* gcc-bugs@gcc.gnu.org.

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



------- Additional Comments From ebotcazou at gcc dot gnu dot org  2003-10-30 13:48 -------
I won't close the PR until the latent problem is fixed on mainline or a specific
decision is made about it, because this is a potential regression in 3.4 wrt to
3.1.x-3.3.x.

As for your testcase: I'll first try to derive a version that fails on mainline
(it's not the case because the generated code is somewhat different on
mainline). If I don't manage to do so, I'll propose your original version.

[Bug optimization/12799] [3.4 regression] faulty mov->add change clobbers the CC register

[cvs-commit at gcc dot gnu dot org]

PLEASE REPLY TO gcc-bugzilla@gcc.gnu.org ONLY, *NOT* gcc-bugs@gcc.gnu.org.

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



------- Additional Comments From cvs-commit at gcc dot gnu dot org  2003-11-02 08:32 -------
Subject: Bug 12799

CVSROOT:	/cvs/gcc
Module name:	gcc
Changes by:	ebotcazou@gcc.gnu.org	2003-11-02 08:32:24

Modified files:
	gcc            : ChangeLog postreload.c 
	gcc/testsuite  : ChangeLog 
Added files:
	gcc/testsuite/gcc.dg: 20031102-1.c 

Log message:
	PR optimization/12799
	* postreload.c (reload_cse_move2add): Generate the add2
	patterns manually.

Patches:
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/ChangeLog.diff?cvsroot=gcc&r1=2.1650&r2=2.1651
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/postreload.c.diff?cvsroot=gcc&r1=2.6&r2=2.7
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/ChangeLog.diff?cvsroot=gcc&r1=1.3162&r2=1.3163
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/gcc.dg/20031102-1.c.diff?cvsroot=gcc&r1=NONE&r2=1.1

[Bug optimization/12799] [3.4 regression] faulty mov->add change clobbers the CC register

[ebotcazou at gcc dot gnu dot org]

PLEASE REPLY TO gcc-bugzilla@gcc.gnu.org ONLY, *NOT* gcc-bugs@gcc.gnu.org.

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


ebotcazou at gcc dot gnu dot org changed:

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


------- Additional Comments From ebotcazou at gcc dot gnu dot org  2003-11-02 08:34 -------
See http://gcc.gnu.org/ml/gcc-patches/2003-11/msg00028.html