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