From patchwork Mon Feb 8 12:25:37 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Kyrill Tkachov X-Patchwork-Id: 580274 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]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 2F5671409B7 for ; Mon, 8 Feb 2016 23:25:54 +1100 (AEDT) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b=V2WNv+sT; dkim-atps=neutral DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :message-id:date:from:mime-version:to:cc:subject:references :in-reply-to:content-type; q=dns; s=default; b=w/kUrQWNN76SshXrF n0tnaW9qSchxjXhXAcGR8qLvF7/yCcS4A744uuu0jRNUkddCza5/B5tKgdxqIBme B3RHFGwDgSY9rEKn+ncF46Mrf/fRTaXaUXrZZltKeyET0S5HnRyrXUnGvEwg6CBq xinO9NADe5kVaSy5v7LtnzHi/8= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :message-id:date:from:mime-version:to:cc:subject:references :in-reply-to:content-type; s=default; bh=ipul+Um/PYNVhDstjBDmWG6 cuiQ=; b=V2WNv+sT3yfT95R9uYkpNTZKb+2VLT8aTf2s6bs8tSN2WW8vGGJkevw 6HeEbJDsU3fM3tKgmgwYAHOs7L29yi/SmUvXwKSZWXaT6FTMvHI4SLGGjgY3gDgL f0UHifzmyqMTAviFteLybhAC3G0PnOAjeJKuQE8frQfv9/op3LSQ= Received: (qmail 9736 invoked by alias); 8 Feb 2016 12:25:44 -0000 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 Received: (qmail 9724 invoked by uid 89); 8 Feb 2016 12:25:43 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-0.9 required=5.0 tests=BAYES_00, KAM_LAZY_DOMAIN_SECURITY, KAM_LOTSOFHASH, RP_MATCHES_RCVD autolearn=no version=3.3.2 spammy=quality X-HELO: foss.arm.com Received: from foss.arm.com (HELO foss.arm.com) (217.140.101.70) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 08 Feb 2016 12:25:42 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.72.51.249]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id B710949; Mon, 8 Feb 2016 04:24:53 -0800 (PST) Received: from [10.2.206.200] (e100706-lin.cambridge.arm.com [10.2.206.200]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 2381D3F25F; Mon, 8 Feb 2016 04:25:38 -0800 (PST) Message-ID: <56B88941.4020707@foss.arm.com> Date: Mon, 08 Feb 2016 12:25:37 +0000 From: Kyrill Tkachov User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.2.0 MIME-Version: 1.0 To: James Greenhalgh CC: GCC Patches , Marcus Shawcroft , Richard Earnshaw Subject: Re: [PATCH][AArch64] PR target/69161: Don't use special predicate for CCmode comparisons in expressions that require matching modes References: <56AB76D6.5020104@foss.arm.com> <20160204134905.GA24322@arm.com> In-Reply-To: <20160204134905.GA24322@arm.com> Hi James, On 04/02/16 13:49, James Greenhalgh wrote: > On Fri, Jan 29, 2016 at 02:27:34PM +0000, Kyrill Tkachov wrote: >> Hi all, >> >> In this PR we ICE during combine when trying to propagate a comparison into a vec_duplicate, >> that is we end up creating the rtx: >> (vec_duplicate:V4SI (eq:CC_NZ (reg:CC_NZ 66 cc) >> (const_int 0 [0]))) >> >> The documentation for vec_duplicate says: >> "The output vector mode must have the same submodes as the input vector mode or the scalar modes" >> So this is invalid RTL, which triggers an assert in simplify-rtx to that effect. >> >> It has been suggested on the PR that this is because we use a special_predicate for >> aarch64_comparison_operator which means that it ignores the mode when matching. >> This is fine when used in RTXes that don't need it, like if_then_else expressions >> but can cause trouble when used in places where the modes do matter, like in >> SET operations. In this particular ICE the cause was the conditional store >> patterns that could end up matching an intermediate rtx during combine of >> (set (reg:SI) (eq:CC_NZ x y)). >> >> The suggested solution is to define a separate predicate with the same >> conditions as aarch64_comparison_operator but make it not special, so it gets >> automatic mode checks to prevent such a situation. >> >> This patch does that. >> Bootstrapped and tested on aarch64-linux-gnu. >> SPEC2006 codegen did not change with this patch, so there shouldn't be >> any code quality regressions. >> >> Ok for trunk? > It would be good to leave a more detailed comment on > "aarch64_comparison_operator_mode" as to why we need it. > > Otherwise, this is OK. Ok, here's the patch with a comment added. I'll commit it when the arm version is approved as well. Thanks, Kyrill > Thanks, > James > >> Thanks, >> Kyrill >> >> 2016-01-29 Kyrylo Tkachov >> >> PR target/69161 >> * config/aarch64/predicates.md (aarch64_comparison_operator_mode): >> New predicate. >> (aarch64_comparison_operator): Break overly long line into two. >> (aarch64_comparison_operation): Likewise. >> * config/aarch64/aarch64.md (cstorecc4): Use >> aarch64_comparison_operator_mode instead of >> aarch64_comparison_operator. >> (cstore4): Likewise. >> (aarch64_cstore): Likewise. >> (*cstoresi_insn_uxtw): Likewise. >> (cstore_neg): Likewise. >> (*cstoresi_neg_uxtw): Likewise. >> >> 2016-01-29 Kyrylo Tkachov >> >> PR target/69161 >> * gcc.c-torture/compile/pr69161.c: New test. diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md index 221f106430ea2c4a44352d937e660d0c1bbe10da..bf2140c5fd9458476d42e49100347dd05e1b21df 100644 --- a/gcc/config/aarch64/aarch64.md +++ b/gcc/config/aarch64/aarch64.md @@ -3039,7 +3039,7 @@ (define_expand "cstore4" (define_expand "cstorecc4" [(set (match_operand:SI 0 "register_operand") - (match_operator 1 "aarch64_comparison_operator" + (match_operator 1 "aarch64_comparison_operator_mode" [(match_operand 2 "cc_register") (match_operand 3 "const0_operand")]))] "" @@ -3051,7 +3051,7 @@ (define_expand "cstorecc4" (define_expand "cstore4" [(set (match_operand:SI 0 "register_operand" "") - (match_operator:SI 1 "aarch64_comparison_operator" + (match_operator:SI 1 "aarch64_comparison_operator_mode" [(match_operand:GPF 2 "register_operand" "") (match_operand:GPF 3 "aarch64_fp_compare_operand" "")]))] "" @@ -3064,7 +3064,7 @@ (define_expand "cstore4" (define_insn "aarch64_cstore" [(set (match_operand:ALLI 0 "register_operand" "=r") - (match_operator:ALLI 1 "aarch64_comparison_operator" + (match_operator:ALLI 1 "aarch64_comparison_operator_mode" [(match_operand 2 "cc_register" "") (const_int 0)]))] "" "cset\\t%0, %m1" @@ -3109,7 +3109,7 @@ (define_insn_and_split "*compare_cstore_insn" (define_insn "*cstoresi_insn_uxtw" [(set (match_operand:DI 0 "register_operand" "=r") (zero_extend:DI - (match_operator:SI 1 "aarch64_comparison_operator" + (match_operator:SI 1 "aarch64_comparison_operator_mode" [(match_operand 2 "cc_register" "") (const_int 0)])))] "" "cset\\t%w0, %m1" @@ -3118,7 +3118,7 @@ (define_insn "*cstoresi_insn_uxtw" (define_insn "cstore_neg" [(set (match_operand:ALLI 0 "register_operand" "=r") - (neg:ALLI (match_operator:ALLI 1 "aarch64_comparison_operator" + (neg:ALLI (match_operator:ALLI 1 "aarch64_comparison_operator_mode" [(match_operand 2 "cc_register" "") (const_int 0)])))] "" "csetm\\t%0, %m1" @@ -3129,7 +3129,7 @@ (define_insn "cstore_neg" (define_insn "*cstoresi_neg_uxtw" [(set (match_operand:DI 0 "register_operand" "=r") (zero_extend:DI - (neg:SI (match_operator:SI 1 "aarch64_comparison_operator" + (neg:SI (match_operator:SI 1 "aarch64_comparison_operator_mode" [(match_operand 2 "cc_register" "") (const_int 0)]))))] "" "csetm\\t%w0, %m1" diff --git a/gcc/config/aarch64/predicates.md b/gcc/config/aarch64/predicates.md index e80e40683cada374303ea987017f95654531a032..11868278c3d0a1887c2065568f890c3eb8ff7f0b 100644 --- a/gcc/config/aarch64/predicates.md +++ b/gcc/config/aarch64/predicates.md @@ -229,10 +229,19 @@ (define_predicate "aarch64_reg_or_imm" ;; True for integer comparisons and for FP comparisons other than LTGT or UNEQ. (define_special_predicate "aarch64_comparison_operator" - (match_code "eq,ne,le,lt,ge,gt,geu,gtu,leu,ltu,unordered,ordered,unlt,unle,unge,ungt")) + (match_code "eq,ne,le,lt,ge,gt,geu,gtu,leu,ltu,unordered, + ordered,unlt,unle,unge,ungt")) + +;; Same as aarch64_comparison_operator but don't ignore the mode. +;; RTL SET operations require their operands source and destination have +;; the same modes, so we can't ignore the modes there. See PR target/69161. +(define_predicate "aarch64_comparison_operator_mode" + (match_code "eq,ne,le,lt,ge,gt,geu,gtu,leu,ltu,unordered, + ordered,unlt,unle,unge,ungt")) (define_special_predicate "aarch64_comparison_operation" - (match_code "eq,ne,le,lt,ge,gt,geu,gtu,leu,ltu,unordered,ordered,unlt,unle,unge,ungt") + (match_code "eq,ne,le,lt,ge,gt,geu,gtu,leu,ltu,unordered, + ordered,unlt,unle,unge,ungt") { if (XEXP (op, 1) != const0_rtx) return false; diff --git a/gcc/testsuite/gcc.c-torture/compile/pr69161.c b/gcc/testsuite/gcc.c-torture/compile/pr69161.c new file mode 100644 index 0000000000000000000000000000000000000000..fdbb63f3335851c2d1537b098f245a9b85ef3695 --- /dev/null +++ b/gcc/testsuite/gcc.c-torture/compile/pr69161.c @@ -0,0 +1,19 @@ +/* PR target/69161. */ + +char a; +int b, c, d, e; + +void +foo (void) +{ + int f; + for (f = 0; f <= 4; f++) + { + for (d = 0; d < 20; d++) + { + __INTPTR_TYPE__ g = (__INTPTR_TYPE__) & c; + b &= (0 != g) > e; + } + e &= a; + } +}