diff mbox

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

Message ID 4E6501D5.6030002@codesourcery.com
State New
Headers show

Commit Message

Andrew Stubbs Sept. 5, 2011, 5:07 p.m. UTC 
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

Comments

Ramana Radhakrishnan Sept. 7, 2011, 2:39 p.m. UTC | #1 
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
>
>
diff mbox

Patch

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