diff mbox

[ARM] Fix broken shift patterns

Message ID 4E8EE285.8080105@codesourcery.com
State New
Headers show

Commit Message

Andrew Stubbs Oct. 7, 2011, 11:29 a.m. UTC 
On 06/10/11 18:17, Paul Brook wrote:
>> I believe this patch to be nothing but an improvement over the current
>> state, and that a fix to the constraint problem should be a separate patch.
>>
>> In that basis, am I OK to commit?
>
> One minor nit:
>
>> (define_special_predicate "shift_operator"
>> ...
>> +	(ior (match_test "GET_CODE (XEXP (op, 1)) == CONST_INT
>> +			&&  ((unsigned HOST_WIDE_INT) INTVAL (XEXP (op, 1)))<  32")
>> +		(match_test "REG_P (XEXP (op, 1))"))))
>
> We're already enforcing the REG_P elsewhere, and it's only valid in some
> contexts, so I'd change this to:
>      (match_test "GET_CODE (XEXP (op, 1)) != CONST_INT
> 	|| ((unsigned HOST_WIDE_INT) INTVAL (XEXP (op, 1)))<  32")

Done, and attached.

> 3. Consistently accept both power-of-two and 0..31 for shifts.  Large shift
> counts give undefined results[1], so replace them with an arbitrary value
> (e.g. 0) during assembly output.  Argualy not an entirely "proper" fix, but I
> think it'll keep everything happy.

I think we need to be careful not to change the behaviour between 
different optimization levels and/or perturbations caused by minor code 
changes. I know this isn't a hard requirement for undefined behaviour, 
but I think it's still good practice.

In this case, I believe the hardware simply shifts the the value clean 
out of the register, and always returns a zero (or maybe -1 for 
ashiftrt?). I'm not sure what it does for rotate.

Anyway, my point is that I don't think that we could insert an immediate 
that had the same effect in all cases.

> For bonus points we should probably disallow MULT in the arm_shiftsi3 pattern,
> stop it interacting with the regulat mulsi3 pattern in undesirable ways.

How might that be a problem? Is it not the case that canonical forms 
already deals with this?

Anyway, it's easily achieved with an extra predicate.

Andrew

Comments

Paul Brook Oct. 7, 2011, 12:37 p.m. UTC | #1 
> Done, and attached.

Ok.

Two changes to the testcase that I'll pre-approve:
- Add a comment along the lines of
  /* ARM has shift-and-alu insns.  Depending on the ALU op GCC represents some
   of these as a left shift, others as a multiply.  Check that we match the
   right one.  */
- Also test (a * 64) - b [rsb] and ~(a * 64) [mvn]

> > 3. Consistently accept both power-of-two and 0..31 for shifts.  Large
> > shift counts give undefined results[1], so replace them with an
> > arbitrary value (e.g. 0) during assembly output.  Argualy not an
> > entirely "proper" fix, but I think it'll keep everything happy.
> 
> I think we need to be careful not to change the behaviour between
> different optimization levels and/or perturbations caused by minor code
> changes. I know this isn't a hard requirement for undefined behaviour,
> but I think it's still good practice.

> In this case, I believe the hardware simply shifts the the value clean
> out of the register, and always returns a zero (or maybe -1 for
> ashiftrt?).

I'm not convinced.  ARM instructions shift modulo 256
(i.e. a >> (b & 0xff).  I'm pretty sure anything that gets constant-folded by 
earlier passes will not honor these semantics.
 
> > For bonus points we should probably disallow MULT in the arm_shiftsi3
> > pattern, stop it interacting with the regular mulsi3 pattern in
> > undesirable ways.
> 
> How might that be a problem? Is it not the case that canonical forms
> already deals with this?

Mainly just general principle that having two insns with the same pattern is 
wrong - reload can't flip between them like it can different altrnatives, and 
there's obscure rules about which one wins when both match.

In this case the shiftsi variant is very restricted and should never occur in 
the first place so it probably doesn't matter.

Paul
diff mbox

Patch

2011-10-07  Andrew Stubbs  <ams@codesourcery.com>

	gcc/
	* config/arm/predicates.md (shift_amount_operand): Remove constant
	range check.
	(shift_operator): Check range of constants for all shift operators.

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

--- a/gcc/config/arm/predicates.md
+++ b/gcc/config/arm/predicates.md
@@ -129,11 +129,12 @@ 
   (ior (match_operand 0 "arm_rhs_operand")
        (match_operand 0 "memory_operand")))
 
+;; This doesn't have to do much because the constant is already checked
+;; in the shift_operator predicate.
 (define_predicate "shift_amount_operand"
   (ior (and (match_test "TARGET_ARM")
 	    (match_operand 0 "s_register_operand"))
-       (and (match_code "const_int")
-	    (match_test "((unsigned HOST_WIDE_INT) INTVAL (op)) < 32"))))
+       (match_operand 0 "const_int_operand")))
 
 (define_predicate "arm_add_operand"
   (ior (match_operand 0 "arm_rhs_operand")
@@ -219,13 +220,20 @@ 
        (match_test "mode == GET_MODE (op)")))
 
 ;; True for shift operators.
+;; Notes:
+;;  * mult is only permitted with a constant shift amount
+;;  * patterns that permit register shift amounts only in ARM mode use
+;;    shift_amount_operand, patterns that always allow registers do not,
+;;    so we don't have to worry about that sort of thing here.
 (define_special_predicate "shift_operator"
   (and (ior (ior (and (match_code "mult")
 		      (match_test "power_of_two_operand (XEXP (op, 1), mode)"))
 		 (and (match_code "rotate")
 		      (match_test "GET_CODE (XEXP (op, 1)) == CONST_INT
 				   && ((unsigned HOST_WIDE_INT) INTVAL (XEXP (op, 1))) < 32")))
-	    (match_code "ashift,ashiftrt,lshiftrt,rotatert"))
+	    (and (match_code "ashift,ashiftrt,lshiftrt,rotatert")
+		 (match_test "GET_CODE (XEXP (op, 1)) != CONST_INT
+			      || ((unsigned HOST_WIDE_INT) INTVAL (XEXP (op, 1))) < 32")))
        (match_test "mode == GET_MODE (op)")))
 
 ;; True for shift operators which can be used with saturation instructions.
--- /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" } */
+}
--- /dev/null
+++ b/gcc/testsuite/gcc.target/arm/shiftable.c
@@ -0,0 +1,43 @@ 
+/* { dg-do compile } */
+/* { dg-options "-O2" } */
+/* { dg-require-effective-target arm32 } */
+
+int
+plus (int a, int b)
+{
+  return (a * 64) + b;
+}
+
+/* { dg-final { scan-assembler "add.*\[al]sl #6" } } */
+
+int
+minus (int a, int b)
+{
+  return a - (b * 64);
+}
+
+/* { dg-final { scan-assembler "sub.*\[al]sl #6" } } */
+
+int
+ior (int a, int b)
+{
+  return (a * 64) | b;
+}
+
+/* { dg-final { scan-assembler "orr.*\[al]sl #6" } } */
+
+int
+xor (int a, int b)
+{
+  return (a * 64) ^ b;
+}
+
+/* { dg-final { scan-assembler "eor.*\[al]sl #6" } } */
+
+int
+and (int a, int b)
+{
+  return (a * 64) & b;
+}
+
+/* { dg-final { scan-assembler "and.*\[al]sl #6" } } */