Patchwork [SPARC] Fix PR target/60941

login
register
mail settings
Submitter Eric Botcazou
Date April 25, 2014, 10:47 a.m.
Message ID <3316944.sc4MgGFZdG@polaris>
Download mbox | patch
Permalink /patch/342757/
State New
Headers show

Comments

Eric Botcazou - April 25, 2014, 10:47 a.m.
This is a regression present on all active branches.  The first pattern added 
by http://gcc.gnu.org/ml/gcc-patches/2011-10/msg00060.html is wrong since, as 
counter-intuitive as it may seem, "sll" also does a full 64-bit shift.

Tested on SPARC/Solaris, applied on all active branches.


2014-04-25  Eric Botcazou  <ebotcazou@adacore.com>

	PR target/60941
	* config/sparc/sparc.md (ashlsi3_extend): Delete.


2014-04-25  Eric Botcazou  <ebotcazou@adacore.com>

	* gcc.c-torture/execute/20140425-1.c: New test.
Jakub Jelinek - April 25, 2014, 10:59 a.m.
On Fri, Apr 25, 2014 at 12:47:32PM +0200, Eric Botcazou wrote:
> /* PR target/60941 */
> /* Reported by Martin Husemann <martin@netbsd.org> */
> 
> extern void abort (void);
> 
> static void __attribute__((noinline))
> set (unsigned long *l)
> {
>   *l = 31;
> }
> 
> int main (void)
> {
>   unsigned long l;
>   int i;
> 
>   set (&l);
>   i = (int) l;
>   l = (unsigned long)(2U << i);
>   if (l != 0)
>     abort ();
>   return 0;
> }

I'm afraid the testcase will fail on int16 targets, so perhaps
you should guard the body of main other than return 0; with
#if __INT_MAX__ >= 2147483647ULL
or
#if __SIZEOF_INT__ >= 4
or similar (or require int32plus target, but gcc.c-torture/execute/
is not a good place for that and *.x files are terribly ugly, so
better would be to move the test to gcc.dg/torture/ in that case).

	Jakub
Eric Botcazou - April 25, 2014, 11:24 a.m.
> I'm afraid the testcase will fail on int16 targets, so perhaps
> you should guard the body of main other than return 0; with
> #if __INT_MAX__ >= 2147483647ULL
> or
> #if __SIZEOF_INT__ >= 4
> or similar (or require int32plus target, but gcc.c-torture/execute/
> is not a good place for that and *.x files are terribly ugly, so
> better would be to move the test to gcc.dg/torture/ in that case).

Not clear to me, (2U << i) should be zero if the shift count is masked.
Jakub Jelinek - April 25, 2014, 11:29 a.m.
On Fri, Apr 25, 2014 at 01:24:13PM +0200, Eric Botcazou wrote:
> > I'm afraid the testcase will fail on int16 targets, so perhaps
> > you should guard the body of main other than return 0; with
> > #if __INT_MAX__ >= 2147483647ULL
> > or
> > #if __SIZEOF_INT__ >= 4
> > or similar (or require int32plus target, but gcc.c-torture/execute/
> > is not a good place for that and *.x files are terribly ugly, so
> > better would be to move the test to gcc.dg/torture/ in that case).
> 
> Not clear to me, (2U << i) should be zero if the shift count is masked.

2U << 31 is undefined behavior on those targets.

	Jakub
Eric Botcazou - April 26, 2014, 1:30 p.m.
> > Not clear to me, (2U << i) should be zero if the shift count is masked.
> 
> 2U << 31 is undefined behavior on those targets.

Precisely not, or else we are not talking about the same notion of masking.
Mikael Pettersson - April 26, 2014, 2:31 p.m.
Eric Botcazou writes:
 > > > Not clear to me, (2U << i) should be zero if the shift count is masked.
 > > 
 > > 2U << 31 is undefined behavior on those targets.
 > 
 > Precisely not, or else we are not talking about the same notion of masking.

I believe Jakub is referring to the following in the C standard:

"Bitwise shift operators
...
Semantics
...  If the value of the right operand ... is greater than or equal to the
width of the promoted left operand, the behavior is undefined."

So on 16-bit int systems you can't portably shift 2U by more than 15.

(I'm not worried about the HW blowing up, but GCC itself has a long history
of exploiting no-undefinedness assumptions.)

/Mikael
Jakub Jelinek - April 26, 2014, 3:02 p.m.
On Sat, Apr 26, 2014 at 03:30:25PM +0200, Eric Botcazou wrote:
> > > Not clear to me, (2U << i) should be zero if the shift count is masked.
> > 
> > 2U << 31 is undefined behavior on those targets.
> 
> Precisely not, or else we are not talking about the same notion of masking.

Eh, C99, 6.5.7/3:
"If the value of the right operand is negative or is
greater than or equal to the width of the promoted left operand, the
behavior is undefined."

So, if you have int16 target, where unsigned int is 16-bit and UINT_MAX 65535,
then shift count must be >= 0 and < 16, therefore, 2U << 31 is undefined
behavior.

	Jakub
Eric Botcazou - April 26, 2014, 6:33 p.m.
> So, if you have int16 target, where unsigned int is 16-bit and UINT_MAX
> 65535, then shift count must be >= 0 and < 16, therefore, 2U << 31 is
> undefined behavior.

Well, if the shift count is masked by the target, if will be masked according 
to the width of the register and the result will nevertheless be zero.
Eric Botcazou - April 26, 2014, 6:33 p.m.
> I believe Jakub is referring to the following in the C standard:
> 
> "Bitwise shift operators
> ...
> Semantics
> ...  If the value of the right operand ... is greater than or equal to the
> width of the promoted left operand, the behavior is undefined."
> 
> So on 16-bit int systems you can't portably shift 2U by more than 15.

Sure, but we're talking about targets for which the shift count is masked, not 
about the C standard.

Patch

Index: config/sparc/sparc.md
===================================================================
--- config/sparc/sparc.md	(revision 209778)
+++ config/sparc/sparc.md	(working copy)
@@ -5795,19 +5795,6 @@  (define_insn "ashlsi3"
 }
   [(set_attr "type" "shift")])
 
-(define_insn "*ashlsi3_extend"
-  [(set (match_operand:DI 0 "register_operand" "=r")
-	(zero_extend:DI
-	  (ashift:SI (match_operand:SI 1 "register_operand" "r")
-		     (match_operand:SI 2 "arith_operand" "rI"))))]
-  "TARGET_ARCH64"
-{
-  if (GET_CODE (operands[2]) == CONST_INT)
-    operands[2] = GEN_INT (INTVAL (operands[2]) & 0x1f);
-  return "sll\t%1, %2, %0";
-}
-  [(set_attr "type" "shift")])
-
 (define_expand "ashldi3"
   [(set (match_operand:DI 0 "register_operand" "=r")
 	(ashift:DI (match_operand:DI 1 "register_operand" "r")