From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 3511 invoked by alias); 27 Jan 2016 17:31:10 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 3474 invoked by uid 89); 27 Jan 2016 17:31:09 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.5 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD,SPF_PASS autolearn=ham version=3.3.2 spammy=wee, duplicates, carry X-HELO: cam-smtp0.cambridge.arm.com Received: from fw-tnat.cambridge.arm.com (HELO cam-smtp0.cambridge.arm.com) (217.140.96.140) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-SHA encrypted) ESMTPS; Wed, 27 Jan 2016 17:31:08 +0000 Received: from arm.com (e107456-lin.cambridge.arm.com [10.2.206.78]) by cam-smtp0.cambridge.arm.com (8.13.8/8.13.8) with ESMTP id u0RHV4WY016331; Wed, 27 Jan 2016 17:31:04 GMT Date: Wed, 27 Jan 2016 17:31:00 -0000 From: James Greenhalgh To: Richard Henderson Cc: gcc-patches@gcc.gnu.org, Marcus Shawcroft , Richard Earnshaw Subject: Re: [PATCH, aarch64] Fix pr69305 -- addti miscompilation Message-ID: <20160127173103.GA9467@arm.com> References: <56A4B347.5020104@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <56A4B347.5020104@redhat.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-IsSubscribed: yes X-SW-Source: 2016-01/txt/msg02143.txt.bz2 On Sun, Jan 24, 2016 at 03:19:35AM -0800, Richard Henderson wrote: > As Jakub notes in the PR, the representation for add_compare and > sub_compare were wrong. And several of the add_carryin patterns > were duplicates. > > This adds a CC_Cmode for which only the Carry bit is valid. > > The patch appears to generate moderately decent code. For gcc7 we > should look into why we'll prefer to mark an output REG_UNUSED > instead of matching the pattern with that output removed. This > results in continuing to use adds (though simplifying adc) after > we've proved that there will be no carry into the high part of an > adds+adc pair. > > Ok? > > > r~ Hi Richard, Some tiny nits below: > diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md > index 71fc514..363785e 100644 > --- a/gcc/config/aarch64/aarch64.md > +++ b/gcc/config/aarch64/aarch64.md > @@ -1755,6 +1755,44 @@ > [(set_attr "type" "alus_sreg,alus_imm,alus_imm")] > ) > > +(define_insn "*add3_compare1_cconly" I don't understand the naming scheme, it got me a wee bit confused with add3_compare0 and friends, where the 0 indicates a comparison with zero... > + [(set (reg:CC_C CC_REGNUM) > + (ne:CC_C > + (plus: > + (zero_extend: > + (match_operand:GPI 0 "aarch64_reg_or_zero" "%rZ,rZ,rZ")) > + (zero_extend: > + (match_operand:GPI 1 "aarch64_plus_operand" "r,I,J"))) > + (zero_extend: > + (plus:GPI (match_dup 0) (match_dup 1)))))] > + "" > + "@ > + cmn\\t%0, %1 > + cmn\\t%0, %1 > + cmp\\t%0, #%n1" > + [(set_attr "type" "alus_sreg,alus_imm,alus_imm")] > +) > + > +(define_insn "add3_compare1" > + [(set (reg:CC_C CC_REGNUM) > + (ne:CC_C > + (plus: > + (zero_extend: > + (match_operand:GPI 1 "aarch64_reg_or_zero" "%rZ,rZ,rZ")) > + (zero_extend: > + (match_operand:GPI 2 "aarch64_plus_operand" "r,I,J"))) > + (zero_extend: > + (plus:GPI (match_dup 1) (match_dup 2))))) > + (set (match_operand:GPI 0 "register_operand" "=r,r,r") > + (plus:GPI (match_dup 1) (match_dup 2)))] > + "" > + "@ > + adds\\t%0, %1, %2 > + adds\\t%0, %1, %2 > + subs\\t%0, %1, #%n2" > + [(set_attr "type" "alus_sreg,alus_imm,alus_imm")] > +) > + > (define_insn "*adds_shift_imm_" > [(set (reg:CC_NZ CC_REGNUM) > (compare:CC_NZ > +;; Note that a single add with carry is matched by cinc, > +;; and the adc_reg and csel types are matched into the same > +;; pipelines by existing cores. I can't see us remembering to update this comment on pipeline models were it to ever become false. Maybe just drop it? > @@ -2440,13 +2427,53 @@ > [(set_attr "type" "alu_ext")] > ) > > -(define_insn "sub3_carryin" > - [(set > - (match_operand:GPI 0 "register_operand" "=r") > - (minus:GPI (minus:GPI > - (match_operand:GPI 1 "register_operand" "r") > - (ltu:GPI (reg:CC CC_REGNUM) (const_int 0))) > - (match_operand:GPI 2 "register_operand" "r")))] > +;; The hardware description is op1 + ~op2 + C. > +;; = op1 + (-op2 + 1) + (1 - !C) > +;; = op1 - op2 - 1 + 1 - !C > +;; = op1 - op2 - !C. > +;; We describe the later. s/later/latter/ Otherwise, this is OK. Thanks, James