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

login
register
mail settings
Submitter Andrew Stubbs
Date Sept. 1, 2011, 11:24 a.m.
Message ID <4E5F6B5F.2020207@codesourcery.com>
Download mbox | patch
Permalink /patch/112875/
State New
Headers show

Comments

Andrew Stubbs - Sept. 1, 2011, 11:24 a.m.
This patch should fix the bug in pr50193.

The problem is that the arith_shiftsi pattern accepted any arbitrary 
constant as the shift amount (via the shift_amount_operand predicate) 
where in fact the constant must be in the range 0..32.

This patch fixes the problem by merely checking that the constant is 
positive. I've confirmed that values larger than the mode-size are not a 
problem because the compiler optimizes those away earlier, even at -O0.

OK?

Andrew
Joseph S. Myers - Sept. 1, 2011, 1:21 p.m.
On Thu, 1 Sep 2011, Andrew Stubbs wrote:

> This patch fixes the problem by merely checking that the constant is positive.
> I've confirmed that values larger than the mode-size are not a problem because
> the compiler optimizes those away earlier, even at -O0.

Do you mean that you have observed for some testcases that they get 
optimized away - or do you have reasons (if so, please state them) to 
believe that any possible path through the compiler that would result in a 
larger constant here (possibly as a result of constant propagation and 
other optimizations) will always result in it being optimized away as 
well?  If it's just observation it would be better to put the complete 
check in here.

Quite of few of the Csmith-generated bug reports from John Regehr have 
involved constants appearing in unexpected places as a result of 
transformations in the compiler.  It would probably be a good idea for 
someone to try using Csmith to find ARM compiler bugs (both ICEs and 
wrong-code); pretty much all the bugs reported have been testing on x86 
and x86_64, so it's likely there are quite a few bugs in the ARM back end 
that could be found that way.
Andrew Stubbs - Sept. 1, 2011, 3:26 p.m.
On 01/09/11 14:21, Joseph S. Myers wrote:
> On Thu, 1 Sep 2011, Andrew Stubbs wrote:
>
>> This patch fixes the problem by merely checking that the constant is positive.
>> I've confirmed that values larger than the mode-size are not a problem because
>> the compiler optimizes those away earlier, even at -O0.
>
> Do you mean that you have observed for some testcases that they get
> optimized away - or do you have reasons (if so, please state them) to
> believe that any possible path through the compiler that would result in a
> larger constant here (possibly as a result of constant propagation and
> other optimizations) will always result in it being optimized away as
> well?  If it's just observation it would be better to put the complete
> check in here.

OK, fair enough, redundant or not, here's a patch with belt and braces.

OK now?

> Quite of few of the Csmith-generated bug reports from John Regehr have
> involved constants appearing in unexpected places as a result of
> transformations in the compiler.  It would probably be a good idea for
> someone to try using Csmith to find ARM compiler bugs (both ICEs and
> wrong-code); pretty much all the bugs reported have been testing on x86
> and x86_64, so it's likely there are quite a few bugs in the ARM back end
> that could be found that way.

Interesting, I've suggested it within Linaro.

Thanks

Andrew

Patch

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

	gcc/
	* config/arm/predicates.md (shift_amount_operand): Ensure shift
	amount is positive.

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

--- a/gcc/config/arm/predicates.md
+++ b/gcc/config/arm/predicates.md
@@ -132,7 +132,8 @@ 
 (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"))))
 
 (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" } */
+}