[Bug c/12654] New: Incorrect comparison code generated for Alpha

[Bug c/12654] New: Incorrect comparison code generated for Alpha

[tg at swox 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=12654

           Summary: Incorrect comparison code generated for Alpha
           Product: gcc
           Version: 3.3
            Status: UNCONFIRMED
          Severity: normal
          Priority: P2
         Component: c
        AssignedTo: unassigned at gcc dot gnu dot org
        ReportedBy: tg at swox dot com
                CC: gcc-bugs at gcc dot gnu dot org
 GCC build triplet: alpha-unknown-freebsd4.5
  GCC host triplet: alpha-unknown-freebsd4.5
GCC target triplet: alpha-unknown-freebsd4.5

No particular compiler options are needed.  Somebody hacking gcc these days
have some serious problems with understanding two's complement arithmetic...!

Test case:

foo (long x)
{
  if (x >= 1024)
    abort ();
}
main ()
{
  foo (~((unsigned long) (~0L) >> 1));
  foo (~((unsigned long) (~0L) >> 1) + 10000);
  exit (0);
}

[Bug c/12654] [3.3/3.4 regression] Incorrect comparison code generated for Alpha

[falk at debian 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=12654


falk at debian dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Severity|normal                      |critical
             Status|UNCONFIRMED                 |NEW
     Ever Confirmed|                            |1
           Keywords|                            |wrong-code
            Summary|Incorrect comparison code   |[3.3/3.4 regression]
                   |generated for Alpha         |Incorrect comparison code
                   |                            |generated for Alpha


------- Additional Comments From falk at debian dot org  2003-10-17 08:52 -------
This boils down to 

foo(MIN_LONG)

(the posted test case is technically invalid).

It is caused by gcc generating

lda     v0,-1023(a0)
cmplt   zero,v0,v0

instead of

lda     v0,1024
cmple   v0,a0,v0

Happens only in some contexts, e.g. for

int foo (long x) { if (x >= 1024) return 1; else return 0; }

but not for

int bar (long x) { return x >= 1024; }

gcc 2.95 got it right, 3.2 not. Probably target dependent, but I don't have 
anything else to test currently.

The bogus variant is actually slightly better WRT register pressure, so we
might want to retain it for int arguments.

[Bug c/12654] [3.3/3.4 regression] Incorrect comparison code generated for Alpha

[tg at swox 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=12654



------- Additional Comments From tg at swox dot com  2003-10-17 15:22 -------
Subject: Re:  [3.3/3.4 regression] Incorrect comparison code generated for Alpha

"falk at debian dot org" <gcc-bugzilla@gcc.gnu.org> writes:

  (the posted test case is technically invalid).
  
The test case is perfectly valid.
The test is the function foo.

In order to drive that portably, a somewhat odd expression is
needed.  Are you saying the ~0L is "technically invalid", since
it contains an overflow of signed data?  Is that relevant for the
test case's validity?  Please take a look at gcc's testsuite for
one million of examples where driver programs for constants that
ate "technically invalid".

I wrote the test in the c-torture style, intended for its execute
category, since I really don't want this sort of bugs to come
back.  We had problems like this back in the gcc 1.x days, and I
am disapppointed we now again have somebody with poor two's
complement understanding checking in code in gcc.

  gcc 2.95 got it right, 3.2 not. Probably target dependent, but I don't have 
  anything else to test currently.
  
Poking around the gcc sources (alpha.c alpha_emit_conditional_branch)
showns that the bogus code has been there for a while.

  The bogus variant is actually slightly better WRT register pressure, so we
  might want to retain it for int arguments.

Huh?  Not sure what you are suggesting.  Adding and then
comarping to 0 is *wrong* also for int arguments when since the
add risks to overflow.

[Bug c/12654] [3.3/3.4 regression] Incorrect comparison code generated for Alpha

[falk dot hueffner at student dot uni-tuebingen dot de]

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

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



------- Additional Comments From falk dot hueffner at student dot uni-tuebingen dot de  2003-10-17 15:47 -------
Subject: Re:  [3.3/3.4 regression] Incorrect comparison code generated for Alpha

"tg at swox dot com" <gcc-bugzilla@gcc.gnu.org> writes:

> In order to drive that portably, a somewhat odd expression is
> needed.  Are you saying the ~0L is "technically invalid", since it
> contains an overflow of signed data?

No, it isn't, since it is an unsigned value. But passing this value to
a function taking a signed value is implementation defined.

> Is that relevant for the test case's validity?

I don't know. I tend to avoid implementation defined behaviour, and in
this case I think MIN_LONG is also a lot clearer.

>   The bogus variant is actually slightly better WRT register pressure, so we
>   might want to retain it for int arguments.
> 
> Huh?  Not sure what you are suggesting.  Adding and then comarping
> to 0 is *wrong* also for int arguments when since the add risks to
> overflow.

Not if you do a 64-bit add. But it's probably not worth bothering.

[Bug c/12654] [3.3/3.4 regression] Incorrect comparison code generated for Alpha

[falk at debian 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=12654


falk at debian dot org changed:

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


------- Additional Comments From falk at debian dot org  2003-10-17 20:52 -------
Created an attachment (id=4952)
 --> (http://gcc.gnu.org/bugzilla/attachment.cgi?id=4952&action=view)
Proposed patch

[Bug target/12654] [3.3/3.4 regression] Incorrect comparison code generated for Alpha

[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=12654


pinskia at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |3.3.3

[Bug target/12654] [3.3/3.4 regression] Incorrect comparison code generated for Alpha

[tg at swox 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=12654



------- Additional Comments From tg at swox dot com  2003-10-17 23:03 -------
Subject: Re:  [3.3/3.4 regression] Incorrect comparison code generated for Alpha

"falk dot hueffner at student dot uni-tuebingen dot de" <gcc-bugzilla@gcc.gnu.org> writes:

  No, it isn't, since it is an unsigned value. But passing this value to
  a function taking a signed value is implementation defined.
  
Passing an "unsigned long" value to a function accepting "long"
is perfectly well-defined, as long as no signed overflow happens
in the conversion.  The value used doesn't cause overflow.

  >   The bogus variant is actually slightly better WRT register pressure, so we
  >   might want to retain it for int arguments.
  > 
  > Huh?  Not sure what you are suggesting.  Adding and then comarping
  > to 0 is *wrong* also for int arguments when since the add risks to
  > overflow.
  
  Not if you do a 64-bit add. But it's probably not worth bothering.
  
Yeah, I suppose you can do the trick in SImode by actually doing
the operations in DImode, so to speak.  Alpha is unusual in that
it allowed larger immediate operands for addition (via lda) than
comparison.

[Bug target/12654] [3.3/3.4 regression] Incorrect comparison code generated for Alpha

[falk dot hueffner at student dot uni-tuebingen dot de]

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

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



------- Additional Comments From falk dot hueffner at student dot uni-tuebingen dot de  2003-10-18 14:05 -------
Subject: Re:  [3.3/3.4 regression] Incorrect comparison code generated for Alpha

"tg at swox dot com" <gcc-bugzilla@gcc.gnu.org> writes:

> While the previous patch correctly addresses the code generation
> problem, it unnecessarily disables an importwnt optimisation for EQ
> and NE comparisions.

Can you please explain this optimization to me? I have trouble seeing
how this transformation would ever enable the bypass.

Also, your patch skips the "Compare and branch against 0 directly" for
unsigned compares; since it doesn't seem to have any effect on the
generated code, it should probably be removed altogether.

[Bug target/12654] [3.3/3.4 regression] Incorrect comparison code generated for Alpha

[tg at swox 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=12654



------- Additional Comments From tg at swox dot com  2003-10-18 14:25 -------
Subject: Re:  [3.3/3.4 regression] Incorrect comparison code generated for Alpha

  Can you please explain this optimization to me? I have trouble
  seeing how this transformation would ever enable the bypass.
  
For (a != IMMEDIATE) and (a == IMMEDIATE) when IMMEDIATE doesn't fit
in a cmpeq but does fit into an lda, we want to generate

        lda     tmp,-IMMEDIATE(a)
        bne/beq ...

instead of

        lda     tmp, IMMEDIATE(r31)
        cmpeq   a, tmp
        beq/bne

I don't understand what you mean with "bypass" in this context.

  Also, your patch skips the "Compare and branch against 0 directly"
  for unsigned compares; since it doesn't seem to have any effect on
  the generated code, it should probably be removed altogether.

I don't understand.

What do you mean by "Compare and branch against 0 directly"?

My change doesn't change the behaviour for unsigned compares, it
merely disables an invalid optimization for signed compares, while the
optimization is left unchanged for EQ/NE compares.

[Bug target/12654] [3.3/3.4 regression] Incorrect comparison code generated for Alpha

[falk dot hueffner at student dot uni-tuebingen dot de]

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

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



------- Additional Comments From falk dot hueffner at student dot uni-tuebingen dot de  2003-10-18 14:58 -------
Subject: Re:  [3.3/3.4 regression] Incorrect comparison code generated for Alpha

"tg at swox dot com" <gcc-bugzilla@gcc.gnu.org> writes:

> For (a != IMMEDIATE) and (a == IMMEDIATE) when IMMEDIATE doesn't fit
> in a cmpeq but does fit into an lda, we want to generate
> 
>         lda     tmp,-IMMEDIATE(a)
>         bne/beq ...
> 
> instead of
> 
>         lda     tmp, IMMEDIATE(r31)
>         cmpeq   a, tmp
>         beq/bne

Okay, I can see that.

> I don't understand what you mean with "bypass" in this context.

The comment in that passage was refering to the EV5 ICMP/ILOG bypass.
Seems the comment is wrong or misleading, but the optimization is
still useful.

>   Also, your patch skips the "Compare and branch against 0 directly"
>   for unsigned compares; since it doesn't seem to have any effect on
>   the generated code, it should probably be removed altogether.
> 
> I don't understand.
> 
> What do you mean by "Compare and branch against 0 directly"?

Uhm, I meant signed. These code lines:

	  /* Whee.  Compare and branch against 0 directly.  */
	  if (op1 == const0_rtx)
	    cmp_code = NIL, branch_code = code;

Previously, they were executed also with eg. LE. It doesn't change the
generated code, though, because this is optimized away somewhere else.

How about this patch, which does the same as yours but seems clearer:

Index: alpha.c
===================================================================
RCS file: /cvsroot/gcc/gcc/gcc/config/alpha/alpha.c,v
retrieving revision 1.332
diff -u -p -c -r1.332 alpha.c
*** alpha.c	11 Oct 2003 16:54:16 -0000	1.332
--- alpha.c	18 Oct 2003 14:52:58 -0000
*************** alpha_emit_conditional_branch (enum rtx_
*** 3150,3176 ****
      {
        cmp_mode = DImode;
  
!       /* The following optimizations are only for signed compares.  */
!       if (code != LEU && code != LTU && code != GEU && code != GTU)
  	{
! 	  /* Whee.  Compare and branch against 0 directly.  */
! 	  if (op1 == const0_rtx)
! 	    cmp_code = NIL, branch_code = code;
  
! 	  /* We want to use cmpcc/bcc when we can, since there is a zero delay
! 	     bypass between logicals and br/cmov on EV5.  But we don't want to
! 	     force valid immediate constants into registers needlessly.  */
! 	  else if (GET_CODE (op1) == CONST_INT)
  	    {
! 	      HOST_WIDE_INT v = INTVAL (op1), n = -v;
! 
! 	      if (! CONST_OK_FOR_LETTER_P (v, 'I')
! 		  && (CONST_OK_FOR_LETTER_P (n, 'K')
! 		      || CONST_OK_FOR_LETTER_P (n, 'L')))
! 		{
! 		  cmp_code = PLUS, branch_code = code;
! 		  op1 = GEN_INT (n);
! 		}
  	    }
  	}
  
--- 3150,3168 ----
      {
        cmp_mode = DImode;
  
!       if ((code == EQ || code == NE) && GET_CODE (op1) == CONST_INT)
  	{
! 	  /* If the constants doesn't fit into an immediate, but can be
! 	     generated by lda/ldah, we adjust the argument and compare
! 	     against zero, so we can use beq/bne directly.  */
! 	  HOST_WIDE_INT v = INTVAL (op1), n = -v;
  
! 	  if (! CONST_OK_FOR_LETTER_P (v, 'I')
! 	      && (CONST_OK_FOR_LETTER_P (n, 'K')
! 		  || CONST_OK_FOR_LETTER_P (n, 'L')))
  	    {
! 	      cmp_code = PLUS, branch_code = code;
! 	      op1 = GEN_INT (n);
  	    }
  	}

[Bug target/12654] [3.3/3.4 regression] Incorrect comparison code generated for Alpha

[tg at swox 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=12654



------- Additional Comments From tg at swox dot com  2003-10-19 05:18 -------
Subject: Re:  [3.3/3.4 regression] Incorrect comparison code generated for Alpha

"falk dot hueffner at student dot uni-tuebingen dot de" <gcc-bugzilla@gcc.gnu.org> writes:

  	  /* Whee.  Compare and branch against 0 directly.  */
  	  if (op1 == const0_rtx)
  	    cmp_code = NIL, branch_code = code;
  
  Previously, they were executed also with eg. LE. It doesn't change the
  generated code, though, because this is optimized away somewhere else.
  
To me, it seems like a bad idea to intensionally generate redundant
insns with the hope they will be optimized away later.

  How about this patch, which does the same as yours but seems clearer:

You comment is better, and surely moving the CONST_INT test
into the main if() is clearer.  But I would argue that my minimalist
change should go in as the fix for this PR.

Any cleanup should be checked in separately, and only on main and
3.4 branches.

[Bug target/12654] [3.3/3.4 regression] Incorrect comparison code generated for Alpha

[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=12654



------- Additional Comments From cvs-commit at gcc dot gnu dot org  2003-10-20 07:59 -------
Subject: Bug 12654

CVSROOT:	/cvs/gcc
Module name:	gcc
Changes by:	falk@gcc.gnu.org	2003-10-20 07:59:46

Modified files:
	gcc            : ChangeLog 
	gcc/config/alpha: alpha.c 
Added files:
	gcc/testsuite/gcc.c-torture/execute: 20031020-1.c 

Log message:
	PR target/12654
	* config/alpha/alpha.c (alpha_emit_conditional_branch): Don't do
	comparison against constant by adjusting the argument except for
	EQ and NE.

Patches:
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/gcc.c-torture/execute/20031020-1.c.diff?cvsroot=gcc&r1=NONE&r2=1.1
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/ChangeLog.diff?cvsroot=gcc&r1=2.1481&r2=2.1482
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/config/alpha/alpha.c.diff?cvsroot=gcc&r1=1.333&r2=1.334

[Bug target/12654] [3.3/3.4 regression] Incorrect comparison code generated for Alpha

[falk at debian 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=12654


falk at debian dot org changed:

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


------- Additional Comments From falk at debian dot org  2003-10-20 08:05 -------
Fixed in 3.4. Will also commit to 3.3 if it is ever revived.

[Bug target/12654] [3.3/3.4 regression] Incorrect comparison code generated for Alpha

[wilson at specifixinc 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=12654



------- Additional Comments From wilson at specifixinc dot com  2003-10-23 00:17 -------
Subject: Re:  [3.3/3.4 regression] Incorrect comparison
 code generated for Alpha

falk at debian dot org wrote:
> Fixed in 3.4. Will also commit to 3.3 if it is ever revived.

This patch is OK for the 3.3 branch.  It is likely that there will be 
more 3.3.x releases, so please do put it on the 3.3 branch so it won't 
be lost.

[Bug target/12654] [3.3/3.4 regression] Incorrect comparison code generated for Alpha

[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=12654



------- Additional Comments From cvs-commit at gcc dot gnu dot org  2003-10-27 20:42 -------
Subject: Bug 12654

CVSROOT:	/cvs/gcc
Module name:	gcc
Branch: 	gcc-3_3-branch
Changes by:	falk@gcc.gnu.org	2003-10-27 20:42:31

Modified files:
	gcc            : ChangeLog 
	gcc/config/alpha: alpha.c 
Added files:
	gcc/testsuite/gcc.c-torture/execute: 20031020-1.c 

Log message:
	PR target/12654
	* config/alpha/alpha.c (alpha_emit_conditional_branch): Don't do
	comparison against constant by adjusting the argument except for
	EQ and NE.

Patches:
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/gcc.c-torture/execute/20031020-1.c.diff?cvsroot=gcc&only_with_tag=gcc-3_3-branch&r1=NONE&r2=1.1.6.1
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/ChangeLog.diff?cvsroot=gcc&only_with_tag=gcc-3_3-branch&r1=1.16114.2.790&r2=1.16114.2.791
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/config/alpha/alpha.c.diff?cvsroot=gcc&only_with_tag=gcc-3_3-branch&r1=1.282.4.7&r2=1.282.4.8

[Bug target/12654] [3.3/3.4 regression] Incorrect comparison code generated for Alpha

[tg at swox dot com]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 1811 bytes --]

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

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



------- Additional Comments From tg at swox dot com  2003-10-27 22:01 -------
Subject: Re:  [3.3/3.4 regression] Incorrect comparison code generated for Alpha

Uhh.

I am very unhappy about my real email address showing up on the
web or in source code that might appear on the web, as part of a
comment in my test case.  This is abolutely unacceptable.  It
must go away immediately, even if it means editing cvs files.

Use tege@swox.com to refer to me in any place where a spammer
could find the address.  That will put the spammer though a loop.

Now we have nested, redundant tests for the comparison code.
With the installed patch, we have the following:

      /* The following optimizations are only for signed compares.  */
      if (code != LEU && code != LTU && code != GEU && code != GTU)
	{
	  /* Whee.  Compare and branch against 0 directly.  */
	  if (op1 == const0_rtx)
	    cmp_code = NIL, branch_code = code;

	  /* If the constants doesn't fit into an immediate, but can
	     be generated by lda/ldah, we adjust the argument and
	     compare against zero, so we can use beq/bne directly.  */
	  else if (GET_CODE (op1) == CONST_INT && (code == EQ || code == NE))
	    {
	      HOST_WIDE_INT v = INTVAL (op1), n = -v;

	      if (! CONST_OK_FOR_LETTER_P (v, 'I')
		  && (CONST_OK_FOR_LETTER_P (n, 'K')
		      || CONST_OK_FOR_LETTER_P (n, 'L')))
		{
		  cmp_code = PLUS, branch_code = code;
		  op1 = GEN_INT (n);
		}
	    }
	}

This is not good.  The compiler will never do unsigned
comparisons against zero.  Therfore, you've introduced more crud
into the compier with this patch.

Why didn't you just use the simple and correct patch I supplied?

--
Torbjörn

[Bug target/12654] [3.3/3.4 regression] Incorrect comparison code generated for Alpha

[falk dot hueffner at student dot uni-tuebingen dot de]

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

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



------- Additional Comments From falk dot hueffner at student dot uni-tuebingen dot de  2003-10-27 22:59 -------
Subject: Re:  [3.3/3.4 regression] Incorrect comparison code generated for Alpha

"tg at swox dot com" <gcc-bugzilla@gcc.gnu.org> writes:

> This is not good.  The compiler will never do unsigned comparisons
> against zero.

Not sure what you mean. If you mean that we never see (gtu x
(const_int 0)), then that's wrong.

> Why didn't you just use the simple and correct patch I supplied?

Because it yields worse code, for example for

int i_lt_0(int i) { if (i < 0) abort(); }