Re: patch to stop GCC from shifting by a negative number

Re: patch to stop GCC from shifting by a negative number

[Steve Ellcey]

> +  /* You want to truncate to a _what_?  */
> +  if (GET_MODE_CLASS (mode) != MODE_INT
> +      && GET_MODE_CLASS (mode) != MODE_PARTIAL_INT)
> +    abort ();
> +

I ran the test suite with your patch and hit this abort a couple of
times on the PA64 HP-UX platform with tests
gcc.c-torture/execute/20020227-1.c and gcc.c-torture/compile/simd-1.c
using -O2 optmization.

In 20020227-1.c I enter simplify_binary_operation
with

code=LSHIFTRT
mode=SCmode
op0=(const_int -1 [0xffffffffffffffff])
op1=(const_int 32 [0x20])

In simd-1.c I enter simplify_binary_operation
with

code=ASHIFT
mode=V2SImode
op0=(const_int 4294967295 [0xffffffff])
op1=(const_int 32 [0x20])

and in both cases I call trunc_int_for_mode right at the end of
simplify_binary_operation with a mode of either SCmode or V2SImode which
then causes the abort to be hit.

Any ideas on what a good fix for these cases would be?

Steve Ellcey
sje@cup.hp.com

Re: patch to stop GCC from shifting by a negative number

[Geoff Keating]

Steve Ellcey <sje@cup.hp.com> writes:

> > +  /* You want to truncate to a _what_?  */
> > +  if (GET_MODE_CLASS (mode) != MODE_INT
> > +      && GET_MODE_CLASS (mode) != MODE_PARTIAL_INT)
> > +    abort ();
> > +
> 
> I ran the test suite with your patch and hit this abort a couple of
> times on the PA64 HP-UX platform with tests
> gcc.c-torture/execute/20020227-1.c and gcc.c-torture/compile/simd-1.c
> using -O2 optmization.
> 
> In 20020227-1.c I enter simplify_binary_operation
> with
> 
> code=LSHIFTRT
> mode=SCmode
> op0=(const_int -1 [0xffffffffffffffff])
> op1=(const_int 32 [0x20])
> 
> In simd-1.c I enter simplify_binary_operation
> with
> 
> code=ASHIFT
> mode=V2SImode
> op0=(const_int 4294967295 [0xffffffff])
> op1=(const_int 32 [0x20])
> 
> and in both cases I call trunc_int_for_mode right at the end of
> simplify_binary_operation with a mode of either SCmode or V2SImode which
> then causes the abort to be hit.
> 
> Any ideas on what a good fix for these cases would be?

I think the bug must be before this (that is,
simplify_binary_operation shouldn't be called this way) because a
right-shift of a complex number is nonsense.  From context, I think it
should be called with DImode.  I bet this is coming from
simplify_subreg or similar and someone used the wrong mode variable.

-- 
- Geoffrey Keating <geoffk@geoffk.org> <geoffk@redhat.com>

Re: patch to stop GCC from shifting by a negative number

[Richard Henderson]

On Fri, Aug 16, 2002 at 02:08:47PM -0700, Steve Ellcey wrote:
> -       /* Don't attempt bitwise arithmetic on non-integral modes.  */
> -       if (! INTEGRAL_MODE_P (compute_mode))
> +       /* Don't attempt bitwise arithmetic on non-integer modes.  */
> +       if (GET_MODE_CLASS (compute_mode) != MODE_INT
> + 	  && GET_MODE_CLASS (compute_mode) != MODE_PARTIAL_INT)
> +          
>   	{

Please introduce a new macro SCALAR_INT_MODE_P for this test,
and use as needed throughout.  Do not introduce random whitespace.

Ok with that change.


r~

Re: patch to stop GCC from shifting by a negative number

[Steve Ellcey]

Here is a new patch to stop GCC from shifting by a negative number in
trunc_int_for_mode.  It is basically Geoff Keating's patch
(http://gcc.gnu.org/ml/gcc-patches/2002-08/msg00853.html) that ensures
we don't call trunc_int_for_mode with VOIDmode but with a couple of
extra changes in combine.c to stop us from calling it with Vector or
Complex modes either.

With Geoff's original patch I was failing
gcc.c-torture/execute/20020227-1.c and gcc.c-torture/compile/simd-1.c,
with them it passes the test suite with no regressions.

Here is a patch that includes both my changes and Geoff's.

Steve Ellcey
sje@cup.hp.com


2002-08-12  Geoffrey Keating  <geoffk@redhat.com>
	    Steve Ellcey  <sje@cup.hp.com>

	* explow.c (trunc_int_for_mode): Abort when the mode is not
	an integer mode.
	* combine.c (expand_compound_operation): Don't expand Vector
	or Complex modes into shifts.
	(expand_field_assignment): Don't do bitwise arithmatic and
	shifts on Vector or Complex modes.
	(simplify_comparison): Don't call trunc_int_for_mode
	for VOIDmode.
	* recog.c (general_operand): Likewise.
	(immediate_operand): Likewise.
	(nonmemory_operand): Likewise.


*** gcc.orig/gcc/explow.c	Fri Aug 16 12:12:13 2002
--- gcc/gcc/explow.c	Fri Aug 16 12:12:55 2002
*************** trunc_int_for_mode (c, mode)
*** 49,54 ****
--- 49,59 ----
  {
    int width = GET_MODE_BITSIZE (mode);
  
+   /* You want to truncate to a _what_?  */
+   if (GET_MODE_CLASS (mode) != MODE_INT
+       && GET_MODE_CLASS (mode) != MODE_PARTIAL_INT)
+     abort ();
+ 
    /* Canonicalize BImode to 0 and STORE_FLAG_VALUE.  */
    if (mode == BImode)
      return c & 1 ? STORE_FLAG_VALUE : 0;
*** gcc.orig/gcc/combine.c	Fri Aug 16 12:12:17 2002
--- gcc/gcc/combine.c	Fri Aug 16 12:20:23 2002
*************** expand_compound_operation (x)
*** 5634,5639 ****
--- 5634,5646 ----
        if (GET_MODE_SIZE (GET_MODE (XEXP (x, 0))) > UNITS_PER_WORD)
  	return x;
  
+       /* Reject MODEs that aren't MODE_INT or MODE_PARTIAL_INT because
+          turning vector or complex modes into shifts causes problems.  */
+ 
+       if (GET_MODE_CLASS (GET_MODE (XEXP (x, 0))) != MODE_INT
+           && GET_MODE_CLASS (GET_MODE (XEXP (x, 0))) != MODE_PARTIAL_INT)
+         return x;
+ 
        len = GET_MODE_BITSIZE (GET_MODE (XEXP (x, 0)));
        /* If the inner object has VOIDmode (the only way this can happen
  	 is if it is an ASM_OPERANDS), we can't do anything since we don't
*************** expand_compound_operation (x)
*** 5655,5660 ****
--- 5662,5674 ----
  	  || GET_MODE (XEXP (x, 0)) == VOIDmode)
  	return x;
  
+       /* Reject MODEs that aren't MODE_INT or MODE_PARTIAL_INT because
+          turning vector or complex modes into shifts causes problems.  */
+ 
+       if (GET_MODE_CLASS (GET_MODE (XEXP (x, 0))) != MODE_INT
+           && GET_MODE_CLASS (GET_MODE (XEXP (x, 0))) != MODE_PARTIAL_INT)
+         return x;
+ 
        len = INTVAL (XEXP (x, 1));
        pos = INTVAL (XEXP (x, 2));
  
*************** expand_field_assignment (x)
*** 5862,5873 ****
  
        compute_mode = GET_MODE (inner);
  
!       /* Don't attempt bitwise arithmetic on non-integral modes.  */
!       if (! INTEGRAL_MODE_P (compute_mode))
  	{
  	  enum machine_mode imode;
  
! 	  /* Something is probably seriously wrong if this matches.  */
  	  if (! FLOAT_MODE_P (compute_mode))
  	    break;
  
--- 5876,5889 ----
  
        compute_mode = GET_MODE (inner);
  
!       /* Don't attempt bitwise arithmetic on non-integer modes.  */
!       if (GET_MODE_CLASS (compute_mode) != MODE_INT
! 	  && GET_MODE_CLASS (compute_mode) != MODE_PARTIAL_INT)
!          
  	{
  	  enum machine_mode imode;
  
! 	  /* Don't try doing anything for Vector or Integer complex types.  */
  	  if (! FLOAT_MODE_P (compute_mode))
  	    break;
  
*************** simplify_comparison (code, pop0, pop1)
*** 10177,10183 ****
  
        /* Get the constant we are comparing against and turn off all bits
  	 not on in our mode.  */
!       const_op = trunc_int_for_mode (INTVAL (op1), mode);
        op1 = GEN_INT (const_op);
  
        /* If we are comparing against a constant power of two and the value
--- 10193,10201 ----
  
        /* Get the constant we are comparing against and turn off all bits
  	 not on in our mode.  */
!       const_op = INTVAL (op1);
!       if (mode != VOIDmode)
! 	const_op = trunc_int_for_mode (const_op, mode);
        op1 = GEN_INT (const_op);
  
        /* If we are comparing against a constant power of two and the value
*** gcc.orig/gcc/recog.c	Fri Aug 16 12:12:21 2002
--- gcc/gcc/recog.c	Fri Aug 16 12:12:55 2002
*************** general_operand (op, mode)
*** 954,959 ****
--- 954,960 ----
      return 0;
  
    if (GET_CODE (op) == CONST_INT
+       && mode != VOIDmode
        && trunc_int_for_mode (INTVAL (op), mode) != INTVAL (op))
      return 0;
  
*************** immediate_operand (op, mode)
*** 1159,1164 ****
--- 1160,1166 ----
      return 0;
  
    if (GET_CODE (op) == CONST_INT
+       && mode != VOIDmode
        && trunc_int_for_mode (INTVAL (op), mode) != INTVAL (op))
      return 0;
  
*************** nonmemory_operand (op, mode)
*** 1241,1246 ****
--- 1243,1249 ----
  	return 0;
  
        if (GET_CODE (op) == CONST_INT
+ 	  && mode != VOIDmode
  	  && trunc_int_for_mode (INTVAL (op), mode) != INTVAL (op))
  	return 0;
  

Re: patch to stop GCC from shifting by a negative number

[Geoff Keating]

[-- Attachment #1: Type: text/plain, Size: 533 bytes --]

Steve Ellcey <sje@cup.hp.com> writes:

> I tracked down a difference between a cross-compiling GCC and a native
> built GCC to trunc_int_for_mode trying to shift by a negative number
> when the function is getting called by general_operand with a CONST_INT
> rtx and a mode of VOIDmode.

Ugh.  It really shouldn't be called this way; it makes no sense.

In fact, in const_int_operand, there's a guard around the
trunc_int_for_mode call to check it against VOIDmode.

What do you think about a patch like this (very lightly tested)?


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: gcc-notruncvoidformode.patch --]
[-- Type: text/x-patch, Size: 2544 bytes --]

2002-08-12  Geoffrey Keating  <geoffk@redhat.com>

	* explow.c (trunc_int_for_mode): Abort when the mode is not
	an integer mode.
	* combine.c (simplify_comparison): Don't call trunc_int_for_mode
	for VOIDmode.
	* recog.c (general_operand): Likewise.
	(immediate_operand): Likewise.
	(nonmemory_operand): Likewise.

Index: combine.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/combine.c,v
retrieving revision 1.309
diff -p -u -p -r1.309 combine.c
--- combine.c	1 Aug 2002 09:08:38 -0000	1.309
+++ combine.c	12 Aug 2002 23:52:14 -0000
@@ -10177,7 +10177,9 @@ simplify_comparison (code, pop0, pop1)
 
       /* Get the constant we are comparing against and turn off all bits
 	 not on in our mode.  */
-      const_op = trunc_int_for_mode (INTVAL (op1), mode);
+      const_op = INTVAL (op1);
+      if (mode != VOIDmode)
+	const_op = trunc_int_for_mode (const_op, mode);
       op1 = GEN_INT (const_op);
 
       /* If we are comparing against a constant power of two and the value
Index: explow.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/explow.c,v
retrieving revision 1.99
diff -p -u -p -r1.99 explow.c
--- explow.c	3 Aug 2002 20:20:35 -0000	1.99
+++ explow.c	12 Aug 2002 23:52:15 -0000
@@ -49,6 +49,11 @@ trunc_int_for_mode (c, mode)
 {
   int width = GET_MODE_BITSIZE (mode);
 
+  /* You want to truncate to a _what_?  */
+  if (GET_MODE_CLASS (mode) != MODE_INT
+      && GET_MODE_CLASS (mode) != MODE_PARTIAL_INT)
+    abort ();
+
   /* Canonicalize BImode to 0 and STORE_FLAG_VALUE.  */
   if (mode == BImode)
     return c & 1 ? STORE_FLAG_VALUE : 0;
Index: recog.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/recog.c,v
retrieving revision 1.162
diff -p -u -p -r1.162 recog.c
--- recog.c	23 Jul 2002 20:50:59 -0000	1.162
+++ recog.c	12 Aug 2002 23:52:15 -0000
@@ -954,6 +954,7 @@ general_operand (op, mode)
     return 0;
 
   if (GET_CODE (op) == CONST_INT
+      && mode != VOIDmode
       && trunc_int_for_mode (INTVAL (op), mode) != INTVAL (op))
     return 0;
 
@@ -1159,6 +1160,7 @@ immediate_operand (op, mode)
     return 0;
 
   if (GET_CODE (op) == CONST_INT
+      && mode != VOIDmode
       && trunc_int_for_mode (INTVAL (op), mode) != INTVAL (op))
     return 0;
 
@@ -1241,6 +1243,7 @@ nonmemory_operand (op, mode)
 	return 0;
 
       if (GET_CODE (op) == CONST_INT
+	  && mode != VOIDmode
 	  && trunc_int_for_mode (INTVAL (op), mode) != INTVAL (op))
 	return 0;
 

[-- Attachment #3: Type: text/plain, Size: 64 bytes --]


-- 
- Geoffrey Keating <geoffk@geoffk.org> <geoffk@redhat.com>

Re: patch to stop GCC from shifting by a negative number

[Richard Henderson]

On Mon, Aug 12, 2002 at 03:57:10PM -0700, Steve Ellcey wrote:
> -   if (width < HOST_BITS_PER_WIDE_INT)
> +   if (width > 0 && width < HOST_BITS_PER_WIDE_INT)

Not ok.  This implies that we're using trunc_int_for_mode
with a VOIDmode or BLKmode argument.  This is wrong.

Feel free to add an abort for this case here.


r~

patch to stop GCC from shifting by a negative number

[Steve Ellcey]

I tracked down a difference between a cross-compiling GCC and a native
built GCC to trunc_int_for_mode trying to shift by a negative number
when the function is getting called by general_operand with a CONST_INT
rtx and a mode of VOIDmode.

This seems like legal input but I found if mode == VOIDmode then
GET_MODE_BITSIZE returns 0 and width is set to 0 in trunc_int_for_mode,
this leads us to to try and shift sign by -1 and this is undefined
behaviour according to the C standard.  In one case I came out of
trunc_int_for_mode with my original constant (and things were good) and
in the other I came out with zero (causing a compile time failure).

I haven't included a test because I don't have one that fails natively,
only when built as a cross compiler from HP-UX PA to HP-UX IA64.

I am rebuilding and retesting now, OK to check in if all the tests pass?

Steve Ellcey
sje@cup.hp.com



2002-08-12  Steve Ellcey  <sje@cup.hp.com>

	* gcc/explow.c (trunc_int_for_mode): Do not try to shift
	by a negative number.


*** gcc.orig/gcc/explow.c	Mon Aug 12 15:35:33 2002
--- gcc/gcc/explow.c	Mon Aug 12 15:36:06 2002
*************** trunc_int_for_mode (c, mode)
*** 54,60 ****
  
    /* Sign-extend for the requested mode.  */
  
!   if (width < HOST_BITS_PER_WIDE_INT)
      {
        HOST_WIDE_INT sign = 1;
        sign <<= width - 1;
--- 54,60 ----
  
    /* Sign-extend for the requested mode.  */
  
!   if (width > 0 && width < HOST_BITS_PER_WIDE_INT)
      {
        HOST_WIDE_INT sign = 1;
        sign <<= width - 1;