public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* [PATCH][ARM] pr50193: ICE on a | (b << negative-constant)
@ 2011-09-01 11:24 Andrew Stubbs
  2011-09-01 13:21 ` Joseph S. Myers
  0 siblings, 1 reply; 9+ messages in thread
From: Andrew Stubbs @ 2011-09-01 11:24 UTC (permalink / raw)
  To: gcc-patches; +Cc: patches

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

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

[-- Attachment #2: pr50193.patch --]
[-- Type: text/x-patch, Size: 946 bytes --]

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" } */
+}

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH][ARM] pr50193: ICE on a | (b << negative-constant)
  2011-09-01 11:24 [PATCH][ARM] pr50193: ICE on a | (b << negative-constant) Andrew Stubbs
@ 2011-09-01 13:21 ` Joseph S. Myers
  2011-09-01 15:26   ` Andrew Stubbs
  0 siblings, 1 reply; 9+ messages in thread
From: Joseph S. Myers @ 2011-09-01 13:21 UTC (permalink / raw)
  To: Andrew Stubbs; +Cc: gcc-patches, patches

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.

-- 
Joseph S. Myers
joseph@codesourcery.com

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH][ARM] pr50193: ICE on a | (b << negative-constant)
  2011-09-01 13:21 ` Joseph S. Myers
@ 2011-09-01 15:26   ` Andrew Stubbs
  2011-09-01 15:29     ` Andrew Stubbs
  0 siblings, 1 reply; 9+ messages in thread
From: Andrew Stubbs @ 2011-09-01 15:26 UTC (permalink / raw)
  To: Joseph S. Myers; +Cc: gcc-patches, patches

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH][ARM] pr50193: ICE on a | (b << negative-constant)
  2011-09-01 15:26   ` Andrew Stubbs
@ 2011-09-01 15:29     ` Andrew Stubbs
  2011-09-01 15:52       ` Jakub Jelinek
  0 siblings, 1 reply; 9+ messages in thread
From: Andrew Stubbs @ 2011-09-01 15:29 UTC (permalink / raw)
  To: Joseph S. Myers; +Cc: gcc-patches, patches

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

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

[-- Attachment #2: pr50193.patch --]
[-- Type: text/x-patch, Size: 1014 bytes --]

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" } */
+}

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH][ARM] pr50193: ICE on a | (b << negative-constant)
  2011-09-01 15:29     ` Andrew Stubbs
@ 2011-09-01 15:52       ` Jakub Jelinek
  2011-09-01 16:21         ` Andrew Stubbs
  2011-09-07 15:37         ` Richard Earnshaw
  0 siblings, 2 replies; 9+ messages in thread
From: Jakub Jelinek @ 2011-09-01 15:52 UTC (permalink / raw)
  To: Andrew Stubbs; +Cc: Joseph S. Myers, gcc-patches, patches

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH][ARM] pr50193: ICE on a | (b << negative-constant)
  2011-09-01 15:52       ` Jakub Jelinek
@ 2011-09-01 16:21         ` Andrew Stubbs
  2011-09-05 17:07           ` Andrew Stubbs
  2011-09-07 15:37         ` Richard Earnshaw
  1 sibling, 1 reply; 9+ messages in thread
From: Andrew Stubbs @ 2011-09-01 16:21 UTC (permalink / raw)
  To: Jakub Jelinek; +Cc: Joseph S. Myers, gcc-patches, patches

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH][ARM] pr50193: ICE on a | (b << negative-constant)
  2011-09-01 16:21         ` Andrew Stubbs
@ 2011-09-05 17:07           ` Andrew Stubbs
  2011-09-07 15:05             ` Ramana Radhakrishnan
  0 siblings, 1 reply; 9+ messages in thread
From: Andrew Stubbs @ 2011-09-05 17:07 UTC (permalink / raw)
  Cc: Jakub Jelinek, Joseph S. Myers, gcc-patches, patches

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

On 01/09/11 17:21, Andrew Stubbs wrote:
> 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?

OK, no reply, so I'm just going to assume we're dealing with 32-bit 
registers.

Additionally, Richard Sandiford has pointed out that changing the 
predicate such that it is more restrictive than the constraints is a 
problem because reload apparently ignores the predicates under certain 
circumstances. Setting aside that that seems broken and wrong (what's 
the point in a predicate if you're just going to ignore it), this patch 
also creates a new constraint "Pm" that limits the range to match the 
predicate.

Speaking of which, I've limited the constants to the range 1..31 
(inclusive) because a) allowing zero seems like it would be 
counter-productive - it would be better to keep a zero shift as a 
separate operation that can be optimized away (probably not an issue, 
but there it is); and b) allowing zero would produce non-canonical 
assembler (which is not a problem now, but is still best avoided).

I have a bootstrap test running now. Assuming that succeeds, is this ok?

Andrew


[-- Attachment #2: pr50193.patch --]
[-- Type: text/x-patch, Size: 6684 bytes --]

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

	gcc/
	* config/arm/arm.md (*not_shiftsi, *not_shiftsi_compare0,
	*not_shiftsi_compare0_scratch, *cmpsi_shiftsi,
	*cmpsi_shiftsi_swp, *arith_shiftsi, *arith_shiftsi_compare0,
	*arith_shiftsi_compare0_scratch, *sub_shiftsi,
	*sub_shiftsi_compare0, *sub_shiftsi_compare0_scratch): Change
	'M' constraint to 'Pm'.
	* config/arm/constraints.md (Pm): New constraint.
	* config/arm/predicates.md (shift_amount_operand): Ensure that all
	constants satisfy the Pm constraint.

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

--- a/gcc/config/arm/arm.md
+++ b/gcc/config/arm/arm.md
@@ -3669,7 +3669,7 @@
   [(set (match_operand:SI 0 "s_register_operand" "=r,r")
 	(not:SI (match_operator:SI 3 "shift_operator"
 		 [(match_operand:SI 1 "s_register_operand" "r,r")
-		  (match_operand:SI 2 "shift_amount_operand" "M,rM")])))]
+		  (match_operand:SI 2 "shift_amount_operand" "Pm,rPm")])))]
   "TARGET_32BIT"
   "mvn%?\\t%0, %1%S3"
   [(set_attr "predicable" "yes")
@@ -3683,7 +3683,7 @@
 	(compare:CC_NOOV
 	 (not:SI (match_operator:SI 3 "shift_operator"
 		  [(match_operand:SI 1 "s_register_operand" "r,r")
-		   (match_operand:SI 2 "shift_amount_operand" "M,rM")]))
+		   (match_operand:SI 2 "shift_amount_operand" "Pm,rPm")]))
 	 (const_int 0)))
    (set (match_operand:SI 0 "s_register_operand" "=r,r")
 	(not:SI (match_op_dup 3 [(match_dup 1) (match_dup 2)])))]
@@ -3700,7 +3700,7 @@
 	(compare:CC_NOOV
 	 (not:SI (match_operator:SI 3 "shift_operator"
 		  [(match_operand:SI 1 "s_register_operand" "r,r")
-		   (match_operand:SI 2 "shift_amount_operand" "M,rM")]))
+		   (match_operand:SI 2 "shift_amount_operand" "Pm,rPm")]))
 	 (const_int 0)))
    (clobber (match_scratch:SI 0 "=r,r"))]
   "TARGET_32BIT"
@@ -7247,7 +7247,7 @@
 	(compare:CC (match_operand:SI   0 "s_register_operand" "r,r")
 		    (match_operator:SI  3 "shift_operator"
 		     [(match_operand:SI 1 "s_register_operand" "r,r")
-		      (match_operand:SI 2 "shift_amount_operand" "M,rM")])))]
+		      (match_operand:SI 2 "shift_amount_operand" "Pm,rPm")])))]
   "TARGET_32BIT"
   "cmp%?\\t%0, %1%S3"
   [(set_attr "conds" "set")
@@ -7259,7 +7259,7 @@
   [(set (reg:CC_SWP CC_REGNUM)
 	(compare:CC_SWP (match_operator:SI 3 "shift_operator"
 			 [(match_operand:SI 1 "s_register_operand" "r,r")
-			  (match_operand:SI 2 "shift_amount_operand" "M,rM")])
+			  (match_operand:SI 2 "shift_amount_operand" "Pm,rPm")])
 			(match_operand:SI 0 "s_register_operand" "r,r")))]
   "TARGET_32BIT"
   "cmp%?\\t%0, %1%S3"
@@ -8649,7 +8649,7 @@
         (match_operator:SI 1 "shiftable_operator"
           [(match_operator:SI 3 "shift_operator"
              [(match_operand:SI 4 "s_register_operand" "r,r,r,r")
-              (match_operand:SI 5 "shift_amount_operand" "M,M,M,r")])
+              (match_operand:SI 5 "shift_amount_operand" "Pm,Pm,Pm,r")])
            (match_operand:SI 2 "s_register_operand" "rk,rk,r,rk")]))]
   "TARGET_32BIT"
   "%i1%?\\t%0, %2, %4%S3"
@@ -8700,7 +8700,7 @@
 	 (match_operator:SI 1 "shiftable_operator"
 	  [(match_operator:SI 3 "shift_operator"
 	    [(match_operand:SI 4 "s_register_operand" "r,r")
-	     (match_operand:SI 5 "shift_amount_operand" "M,r")])
+	     (match_operand:SI 5 "shift_amount_operand" "Pm,r")])
 	   (match_operand:SI 2 "s_register_operand" "r,r")])
 	 (const_int 0)))
    (set (match_operand:SI 0 "s_register_operand" "=r,r")
@@ -8719,7 +8719,7 @@
 	 (match_operator:SI 1 "shiftable_operator"
 	  [(match_operator:SI 3 "shift_operator"
 	    [(match_operand:SI 4 "s_register_operand" "r,r")
-	     (match_operand:SI 5 "shift_amount_operand" "M,r")])
+	     (match_operand:SI 5 "shift_amount_operand" "Pm,r")])
 	   (match_operand:SI 2 "s_register_operand" "r,r")])
 	 (const_int 0)))
    (clobber (match_scratch:SI 0 "=r,r"))]
@@ -8735,7 +8735,7 @@
 	(minus:SI (match_operand:SI 1 "s_register_operand" "r,r")
 		  (match_operator:SI 2 "shift_operator"
 		   [(match_operand:SI 3 "s_register_operand" "r,r")
-		    (match_operand:SI 4 "shift_amount_operand" "M,r")])))]
+		    (match_operand:SI 4 "shift_amount_operand" "Pm,r")])))]
   "TARGET_32BIT"
   "sub%?\\t%0, %1, %3%S2"
   [(set_attr "predicable" "yes")
@@ -8749,7 +8749,7 @@
 	 (minus:SI (match_operand:SI 1 "s_register_operand" "r,r")
 		   (match_operator:SI 2 "shift_operator"
 		    [(match_operand:SI 3 "s_register_operand" "r,r")
-		     (match_operand:SI 4 "shift_amount_operand" "M,rM")]))
+		     (match_operand:SI 4 "shift_amount_operand" "Pm,rPm")]))
 	 (const_int 0)))
    (set (match_operand:SI 0 "s_register_operand" "=r,r")
 	(minus:SI (match_dup 1)
@@ -8767,7 +8767,7 @@
 	 (minus:SI (match_operand:SI 1 "s_register_operand" "r,r")
 		   (match_operator:SI 2 "shift_operator"
 		    [(match_operand:SI 3 "s_register_operand" "r,r")
-		     (match_operand:SI 4 "shift_amount_operand" "M,rM")]))
+		     (match_operand:SI 4 "shift_amount_operand" "Pm,rPm")]))
 	 (const_int 0)))
    (clobber (match_scratch:SI 0 "=r,r"))]
   "TARGET_32BIT"
--- a/gcc/config/arm/constraints.md
+++ b/gcc/config/arm/constraints.md
@@ -31,7 +31,7 @@
 ;; The following multi-letter normal constraints have been used:
 ;; in ARM/Thumb-2 state: Da, Db, Dc, Dn, Dl, DL, Dv, Dy, Di, Dz
 ;; in Thumb-1 state: Pa, Pb, Pc, Pd
-;; in Thumb-2 state: Pj, PJ, Ps, Pt, Pu, Pv, Pw, Px, Py
+;; in Thumb-2 state: Pj, PJ, Pm, Ps, Pt, Pu, Pv, Pw, Px, Py
 
 ;; The following memory constraints have been used:
 ;; in ARM/Thumb-2 state: Q, Ut, Uv, Uy, Un, Um, Us
@@ -172,6 +172,12 @@
   (and (match_code "const_int")
        (match_test "TARGET_THUMB1 && ival >= 0 && ival <= 7")))
 
+(define_constraint "Pm"
+  "@internal In ARM/Thumb2 state, a constant in the range 1 to 31"
+  (and (match_code "const_int")
+       (match_test "TARGET_32BIT
+		    && IN_RANGE (ival, 1, 31)")))
+
 (define_constraint "Ps"
   "@internal In Thumb-2 state a constant in the range -255 to +255"
   (and (match_code "const_int")
--- 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 "satisfies_constraint_Pm (op)"))))
 
 (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" } */
+}

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH][ARM] pr50193: ICE on a | (b << negative-constant)
  2011-09-05 17:07           ` Andrew Stubbs
@ 2011-09-07 15:05             ` Ramana Radhakrishnan
  0 siblings, 0 replies; 9+ messages in thread
From: Ramana Radhakrishnan @ 2011-09-07 15:05 UTC (permalink / raw)
  To: Andrew Stubbs; +Cc: Jakub Jelinek, Joseph S. Myers, gcc-patches, patches

On 5 September 2011 18:07, Andrew Stubbs <ams@codesourcery.com> wrote:
> On 01/09/11 17:21, Andrew Stubbs wrote:
>>
>> 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?
>
> OK, no reply, so I'm just going to assume we're dealing with 32-bit
> registers.
>
> Additionally, Richard Sandiford has pointed out that changing the predicate
> such that it is more restrictive than the constraints is a problem because
> reload apparently ignores the predicates under certain circumstances.
> Setting aside that that seems broken and wrong (what's the point in a
> predicate if you're just going to ignore it), this patch also creates a new
> constraint "Pm" that limits the range to match the predicate.



>
> Speaking of which, I've limited the constants to the range 1..31 (inclusive)
> because a) allowing zero seems like it would be counter-productive - it
> would be better to keep a zero shift as a separate operation that can be
> optimized away (probably not an issue, but there it is); and b) allowing
> zero would produce non-canonical assembler (which is not a problem now, but
> is still best avoided).
>
> I have a bootstrap test running now. Assuming that succeeds, is this ok?

This is a summary of what we discussed on IRC for others to also comment.

This doesn't capture the case of mult by power_of_two in arith_shiftsi .

So this testcase below

int * foo (int *x , int y)
{
   return x + (y << 24);
}

will fail .


There are 2 options as we discussed on IRC .
1. as you suggested a check in shift_operator for the shift amount.
2. add an alternative for the M constraint but enable it using
insn_enabled only if the mult_operator is valid.

This sort of cries to be rewritten with iterators and then we can
retain the mult case as a specific pattern only with the M constraint
but that is a significant rewrite.

> +;; in Thumb-2 state: Pj, PJ, Pm, Ps, Pt, Pu, Pv, Pw, Px, Py

Minor nit - Pm is valid for both ARM / Thumb2 state and not just
Thumb2 unlike the other constraints in the list above.

Hope this helps.

Ramana

>
> Andrew
>
>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH][ARM] pr50193: ICE on a | (b << negative-constant)
  2011-09-01 15:52       ` Jakub Jelinek
  2011-09-01 16:21         ` Andrew Stubbs
@ 2011-09-07 15:37         ` Richard Earnshaw
  1 sibling, 0 replies; 9+ messages in thread
From: Richard Earnshaw @ 2011-09-07 15:37 UTC (permalink / raw)
  To: Jakub Jelinek; +Cc: Andrew Stubbs, Joseph S. Myers, gcc-patches, patches

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.

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2011-09-07 15:12 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-09-01 11:24 [PATCH][ARM] pr50193: ICE on a | (b << negative-constant) Andrew Stubbs
2011-09-01 13:21 ` Joseph S. Myers
2011-09-01 15:26   ` Andrew Stubbs
2011-09-01 15:29     ` Andrew Stubbs
2011-09-01 15:52       ` Jakub Jelinek
2011-09-01 16:21         ` Andrew Stubbs
2011-09-05 17:07           ` Andrew Stubbs
2011-09-07 15:05             ` Ramana Radhakrishnan
2011-09-07 15:37         ` Richard Earnshaw

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).