[ARM] pr50193: ICE on a | (b << negative-constant)

Submitted by Andrew Stubbs on Sept. 1, 2011, 3:29 p.m.

Details

Message ID 4E5FA4D5.9050309@codesourcery.com
State New
Headers show

Commit Message

Andrew Stubbs Sept. 1, 2011, 3:29 p.m.
On 01/09/11 16:26, Andrew Stubbs wrote:
> OK, fair enough, redundant or not, here's a patch with belt and braces.
>
> OK now?

And again, with the patch ....

Andrew

Comments

Jakub Jelinek Sept. 1, 2011, 3:51 p.m.
On Thu, Sep 01, 2011 at 04:29:25PM +0100, Andrew Stubbs wrote:
> On 01/09/11 16:26, Andrew Stubbs wrote:
> >OK, fair enough, redundant or not, here's a patch with belt and braces.
> >
> >OK now?
> 
> And again, with the patch ....
> 
> Andrew

> 2011-09-01  Andrew Stubbs  <ams@codesourcery.com>
> 
> 	gcc/
> 	* config/arm/predicates.md (shift_amount_operand): Ensure shift
> 	amount is in the range 1..mode_size-1.
> 
> 	gcc/testsuite/
> 	* gcc.dg/pr50193-1.c: New file.
> 
> --- a/gcc/config/arm/predicates.md
> +++ b/gcc/config/arm/predicates.md
> @@ -132,7 +132,9 @@
>  (define_predicate "shift_amount_operand"
>    (ior (and (match_test "TARGET_ARM")
>  	    (match_operand 0 "s_register_operand"))
> -       (match_operand 0 "const_int_operand")))
> +       (and (match_operand 0 "const_int_operand")
> +	    (match_test "INTVAL (op) > 0
> +			 && INTVAL (op) <= GET_MODE_PRECISION (mode)"))))

IN_RANGE (INTVAL (op), 0, GET_MODE_PRECISION (mode) - 1) ?

1) shift by 0 is well defined (though not sure if arm backend
   supports it)
2) shift by mode precision is undefined
3) isn't mode here the mode of the shift operand rather than of the shift
   itself (perhaps on arm in all patterns they are the same - 
   a brief look at arm.amd suggest they are and they are SImode in all
   cases, so perhaps just IN_RANGE (INTVAL (op), 0, 31) ?)

	Jakub
Andrew Stubbs Sept. 1, 2011, 4:21 p.m.
On 01/09/11 16:51, Jakub Jelinek wrote:
> IN_RANGE (INTVAL (op), 0, GET_MODE_PRECISION (mode) - 1) ?

Ok, I can make that change.

> 1) shift by 0 is well defined (though not sure if arm backend
>     supports it)

Yeah, I suppose I could allow 0, but I don't know why it would be 
helpful? I mean, it's not like there's an operation that is only 
available with a no-op shift (I suppose you could say that all the ARM 
opcodes are like that, but neither the md file nor the official 
canonical assembler notation represents it that way).

Conversely, assuming it's even possible for a zero shift to get this 
far, I imagined it would be possible for combine to use the 
arith_shiftsi pattern and therefore prevent it from combining the 
and/or/add/whatever into something else more worthwhile, so I chose to 
insist on greater than zero.

> 2) shift by mode precision is undefined

The LSL and ROR require the shift amount to be 1..31 while the LSR and 
ASR require 1..32. Anecdotally speaking, gcc optimizes away (and warns 
about) shifts greater than 31, so I chose mode-1.

> 3) isn't mode here the mode of the shift operand rather than of the shift
>     itself (perhaps on arm in all patterns they are the same -
>     a brief look at arm.amd suggest they are and they are SImode in all
>     cases, so perhaps just IN_RANGE (INTVAL (op), 0, 31) ?)

Well, I didn't want to hard-code the number, but yes, that would be 
equivalent at present, I think.

I wasn't sure how to find the mode of shift operand in the predicate 
though, so I've assumed they're always the same size.  How would one 
find the proper mode in a predicate?

Andrew
Richard Earnshaw Sept. 7, 2011, 3:12 p.m.
On 01/09/11 16:51, Jakub Jelinek wrote:
> 1) shift by 0 is well defined (though not sure if arm backend
>    supports it)

The canonical form of
	(<any_shift> (x) (const_int 0))

is just

	(x)

So while it's well defined, it's also useless.

Patch hide | download patch | download mbox

2011-09-01  Andrew Stubbs  <ams@codesourcery.com>

	gcc/
	* config/arm/predicates.md (shift_amount_operand): Ensure shift
	amount is in the range 1..mode_size-1.

	gcc/testsuite/
	* gcc.dg/pr50193-1.c: New file.

--- a/gcc/config/arm/predicates.md
+++ b/gcc/config/arm/predicates.md
@@ -132,7 +132,9 @@ 
 (define_predicate "shift_amount_operand"
   (ior (and (match_test "TARGET_ARM")
 	    (match_operand 0 "s_register_operand"))
-       (match_operand 0 "const_int_operand")))
+       (and (match_operand 0 "const_int_operand")
+	    (match_test "INTVAL (op) > 0
+			 && INTVAL (op) <= GET_MODE_PRECISION (mode)"))))
 
 (define_predicate "arm_add_operand"
   (ior (match_operand 0 "arm_rhs_operand")
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr50193-1.c
@@ -0,0 +1,10 @@ 
+/* PR 50193: ARM: ICE on a | (b << negative-constant) */
+/* Ensure that the compiler doesn't ICE.  */
+
+/* { dg-options "-O2" } */
+
+int
+foo(int a, int b)
+{
+  return a | (b << -3); /* { dg-warning "left shift count is negative" } */
+}