From patchwork Fri Oct 7 11:29:09 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andrew Stubbs X-Patchwork-Id: 118305 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) by ozlabs.org (Postfix) with SMTP id 3F794B70B1 for ; Fri, 7 Oct 2011 22:29:34 +1100 (EST) Received: (qmail 10856 invoked by alias); 7 Oct 2011 11:29:32 -0000 Received: (qmail 10844 invoked by uid 22791); 7 Oct 2011 11:29:31 -0000 X-SWARE-Spam-Status: No, hits=-0.3 required=5.0 tests=AWL,BAYES_50 X-Spam-Check-By: sourceware.org Received: from relay1.mentorg.com (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 07 Oct 2011 11:29:16 +0000 Received: from nat-ies.mentorg.com ([192.94.31.2] helo=EU1-MAIL.mgc.mentorg.com) by relay1.mentorg.com with esmtp id 1RC8c7-0001Vu-Lq from Andrew_Stubbs@mentor.com ; Fri, 07 Oct 2011 04:29:15 -0700 Received: from [127.0.0.1] ([172.16.63.104]) by EU1-MAIL.mgc.mentorg.com with Microsoft SMTPSVC(6.0.3790.1830); Fri, 7 Oct 2011 12:29:13 +0100 Message-ID: <4E8EE285.8080105@codesourcery.com> Date: Fri, 07 Oct 2011 12:29:09 +0100 From: Andrew Stubbs User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:7.0) Gecko/20110923 Thunderbird/7.0 MIME-Version: 1.0 To: Paul Brook CC: "gcc-patches@gcc.gnu.org" , richard.sandiford@linaro.org Subject: Re: [PATCH][ARM] Fix broken shift patterns References: <4E8DC2C1.8000103@codesourcery.com> <201110061817.36765.paul@codesourcery.com> In-Reply-To: <201110061817.36765.paul@codesourcery.com> Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org 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 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" } } */