This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH][ARM] Fix broken shift patterns


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

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]