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

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

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

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

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.