Bug 12654

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.

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.

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.

Created attachment 4952 [details]
Proposed patch

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.

Created attachment 4953 [details]
A better patch.

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

My simpler patch is better.

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.

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.

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

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.

Created attachment 4958 [details]
Another patch

Okay, this patch fixes the bug, is simple, and doesn't change any other
behaviour.
Everybody should like it ;) I will post it to gcc-patches as soon as testing
finishes.

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

Fixed in 3.4. Will also commit to 3.3 if it is ever revived.

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.

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

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

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