From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 84877 invoked by alias); 13 Jan 2017 13:50:30 -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 84862 invoked by uid 89); 13 Jan 2017 13:50:29 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-5.1 required=5.0 tests=BAYES_00,RP_MATCHES_RCVD,SPF_PASS autolearn=ham version=3.3.2 spammy= 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; Fri, 13 Jan 2017 13:50:18 +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 1AE15AD7; Fri, 13 Jan 2017 05:50:17 -0800 (PST) Received: from e105689-lin.cambridge.arm.com (e105689-lin.cambridge.arm.com [10.2.207.32]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 91ED43F3D6; Fri, 13 Jan 2017 05:50:15 -0800 (PST) Subject: Re: [PATCH, ARM] correctly encode the CC reg data flow To: Bernd Edlinger , "gcc-patches@gcc.gnu.org" References: Cc: Ramana Radhakrishnan , Kyrill Tkachov , Wilco Dijkstra From: "Richard Earnshaw (lists)" Message-ID: <3f5e5538-5dd3-b416-904f-b87f115336fe@arm.com> Date: Fri, 13 Jan 2017 13:50:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.5.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit X-SW-Source: 2017-01/txt/msg00942.txt.bz2 On 18/12/16 12:58, Bernd Edlinger wrote: > Hi, > > this is related to PR77308, the follow-up patch will depend on this one. > > When trying the split the *arm_cmpdi_insn and *arm_cmpdi_unsigned > before reload, a mis-compilation in libgcc function __gnu_satfractdasq > was discovered, see [1] for more details. > > The reason seems to be that when the *arm_cmpdi_insn is directly > followed by a *arm_cmpdi_unsigned instruction, both are split > up into this: > > [(set (reg:CC CC_REGNUM) > (compare:CC (match_dup 0) (match_dup 1))) > (parallel [(set (reg:CC CC_REGNUM) > (compare:CC (match_dup 3) (match_dup 4))) > (set (match_dup 2) > (minus:SI (match_dup 5) > (ltu:SI (reg:CC_C CC_REGNUM) (const_int > 0))))])] > > [(set (reg:CC CC_REGNUM) > (compare:CC (match_dup 2) (match_dup 3))) > (cond_exec (eq:SI (reg:CC CC_REGNUM) (const_int 0)) > (set (reg:CC CC_REGNUM) > (compare:CC (match_dup 0) (match_dup 1))))] > > The problem is that the reg:CC from the *subsi3_carryin_compare > is not mentioning that the reg:CC is also dependent on the reg:CC > from before. Therefore the *arm_cmpsi_insn appears to be > redundant and thus got removed, because the data values are identical. > > I think that applies to a number of similar pattern where data > flow is happening through the CC reg. > > So this is a kind of correctness issue, and should be fixed > independently from the optimization issue PR77308. > > Therefore I think the patterns need to specify the true > value that will be in the CC reg, in order for cse to > know what the instructions are really doing. > > > Bootstrapped and reg-tested on arm-linux-gnueabihf. > Is it OK for trunk? > I agree you've found a valid problem here, but I have some issues with the patch itself. (define_insn_and_split "subdi3_compare1" [(set (reg:CC_NCV CC_REGNUM) (compare:CC_NCV (match_operand:DI 1 "register_operand" "r") (match_operand:DI 2 "register_operand" "r"))) (set (match_operand:DI 0 "register_operand" "=&r") (minus:DI (match_dup 1) (match_dup 2)))] "TARGET_32BIT" "#" "&& reload_completed" [(parallel [(set (reg:CC CC_REGNUM) (compare:CC (match_dup 1) (match_dup 2))) (set (match_dup 0) (minus:SI (match_dup 1) (match_dup 2)))]) (parallel [(set (reg:CC_C CC_REGNUM) (compare:CC_C (zero_extend:DI (match_dup 4)) (plus:DI (zero_extend:DI (match_dup 5)) (ltu:DI (reg:CC_C CC_REGNUM) (const_int 0))))) (set (match_dup 3) (minus:SI (minus:SI (match_dup 4) (match_dup 5)) (ltu:SI (reg:CC_C CC_REGNUM) (const_int 0))))])] This pattern is now no-longer self consistent in that before the split the overall result for the condition register is in mode CC_NCV, but afterwards it is just CC_C. I think CC_NCV is correct mode (the N, C and V bits all correctly reflect the result of the 64-bit comparison), but that then implies that the cc mode of subsi3_carryin_compare is incorrect as well and should in fact also be CC_NCV. Thinking about this pattern, I'm inclined to agree that CC_NCV is the correct mode for this operation I'm not sure if there are other consequences that will fall out from fixing this (it's possible that we might need a change to select_cc_mode as well). R. > > Thanks > Bernd. > > > [1] https://gcc.gnu.org/ml/gcc-patches/2016-12/msg00680.html > > > patch-pr77308-5.diff > > > 2016-12-10 Bernd Edlinger > > PR target/77308 > * config/arm/arm.md (adddi3_compareV, *addsi3_compareV_upper, > adddi3_compareC, *addsi3_compareC_upper, subdi3_compare1, > subsi3_carryin_compare, subsi3_carryin_compare_const, > negdi2_compare, *negsi2_carryin_compare, > *arm_cmpdi_insn): Fix the CC reg dataflow. > > Index: gcc/config/arm/arm.md > =================================================================== > --- gcc/config/arm/arm.md (revision 243515) > +++ gcc/config/arm/arm.md (working copy) > @@ -669,17 +669,15 @@ > (set (match_dup 0) (plus:SI (match_dup 1) (match_dup 2)))]) > (parallel [(set (reg:CC_V CC_REGNUM) > (ne:CC_V > - (plus:DI (plus:DI > - (sign_extend:DI (match_dup 4)) > - (sign_extend:DI (match_dup 5))) > - (ltu:DI (reg:CC_C CC_REGNUM) (const_int 0))) > - (plus:DI (sign_extend:DI > - (plus:SI (match_dup 4) (match_dup 5))) > - (ltu:DI (reg:CC_C CC_REGNUM) (const_int 0))))) > - (set (match_dup 3) (plus:SI (plus:SI > - (match_dup 4) (match_dup 5)) > - (ltu:SI (reg:CC_C CC_REGNUM) > - (const_int 0))))])] > + (plus:DI (plus:DI (sign_extend:DI (match_dup 4)) > + (sign_extend:DI (match_dup 5))) > + (ltu:DI (reg:CC_C CC_REGNUM) (const_int 0))) > + (sign_extend:DI > + (plus:SI (plus:SI (match_dup 4) (match_dup 5)) > + (ltu:SI (reg:CC_C CC_REGNUM) (const_int 0)))))) > + (set (match_dup 3) (plus:SI (plus:SI (match_dup 4) (match_dup 5)) > + (ltu:SI (reg:CC_C CC_REGNUM) > + (const_int 0))))])] > " > { > operands[3] = gen_highpart (SImode, operands[0]); > @@ -713,13 +711,13 @@ > [(set (reg:CC_V CC_REGNUM) > (ne:CC_V > (plus:DI > - (plus:DI > - (sign_extend:DI (match_operand:SI 1 "register_operand" "r")) > - (sign_extend:DI (match_operand:SI 2 "register_operand" "r"))) > - (ltu:DI (reg:CC_C CC_REGNUM) (const_int 0))) > - (plus:DI (sign_extend:DI > - (plus:SI (match_dup 1) (match_dup 2))) > - (ltu:DI (reg:CC_C CC_REGNUM) (const_int 0))))) > + (plus:DI > + (sign_extend:DI (match_operand:SI 1 "register_operand" "r")) > + (sign_extend:DI (match_operand:SI 2 "register_operand" "r"))) > + (ltu:DI (reg:CC_C CC_REGNUM) (const_int 0))) > + (sign_extend:DI (plus:SI (plus:SI (match_dup 1) (match_dup 2)) > + (ltu:SI (reg:CC_C CC_REGNUM) > + (const_int 0)))))) > (set (match_operand:SI 0 "register_operand" "=r") > (plus:SI > (plus:SI (match_dup 1) (match_dup 2)) > @@ -748,17 +746,15 @@ > (set (match_dup 0) (plus:SI (match_dup 1) (match_dup 2)))]) > (parallel [(set (reg:CC_C CC_REGNUM) > (ne:CC_C > - (plus:DI (plus:DI > - (zero_extend:DI (match_dup 4)) > - (zero_extend:DI (match_dup 5))) > - (ltu:DI (reg:CC_C CC_REGNUM) (const_int 0))) > - (plus:DI (zero_extend:DI > - (plus:SI (match_dup 4) (match_dup 5))) > - (ltu:DI (reg:CC_C CC_REGNUM) (const_int 0))))) > - (set (match_dup 3) (plus:SI > - (plus:SI (match_dup 4) (match_dup 5)) > - (ltu:SI (reg:CC_C CC_REGNUM) > - (const_int 0))))])] > + (plus:DI (plus:DI (zero_extend:DI (match_dup 4)) > + (zero_extend:DI (match_dup 5))) > + (ltu:DI (reg:CC_C CC_REGNUM) (const_int 0))) > + (zero_extend:DI > + (plus:SI (plus:SI (match_dup 4) (match_dup 5)) > + (ltu:SI (reg:CC_C CC_REGNUM) (const_int 0)))))) > + (set (match_dup 3) (plus:SI (plus:SI (match_dup 4) (match_dup 5)) > + (ltu:SI (reg:CC_C CC_REGNUM) > + (const_int 0))))])] > " > { > operands[3] = gen_highpart (SImode, operands[0]); > @@ -777,17 +773,16 @@ > [(set (reg:CC_C CC_REGNUM) > (ne:CC_C > (plus:DI > - (plus:DI > - (zero_extend:DI (match_operand:SI 1 "register_operand" "r")) > - (zero_extend:DI (match_operand:SI 2 "register_operand" "r"))) > - (ltu:DI (reg:CC_C CC_REGNUM) (const_int 0))) > - (plus:DI (zero_extend:DI > - (plus:SI (match_dup 1) (match_dup 2))) > - (ltu:DI (reg:CC_C CC_REGNUM) (const_int 0))))) > + (plus:DI > + (zero_extend:DI (match_operand:SI 1 "register_operand" "r")) > + (zero_extend:DI (match_operand:SI 2 "register_operand" "r"))) > + (ltu:DI (reg:CC_C CC_REGNUM) (const_int 0))) > + (zero_extend:DI > + (plus:SI (plus:SI (match_dup 1) (match_dup 2)) > + (ltu:SI (reg:CC_C CC_REGNUM) (const_int 0)))))) > (set (match_operand:SI 0 "register_operand" "=r") > - (plus:SI > - (plus:SI (match_dup 1) (match_dup 2)) > - (ltu:SI (reg:CC_C CC_REGNUM) (const_int 0))))] > + (plus:SI (plus:SI (match_dup 1) (match_dup 2)) > + (ltu:SI (reg:CC_C CC_REGNUM) (const_int 0))))] > "TARGET_32BIT" > "adcs%?\\t%0, %1, %2" > [(set_attr "conds" "set") > @@ -1086,8 +1081,8 @@ > }) > > (define_insn_and_split "subdi3_compare1" > - [(set (reg:CC CC_REGNUM) > - (compare:CC > + [(set (reg:CC_NCV CC_REGNUM) > + (compare:CC_NCV > (match_operand:DI 1 "register_operand" "r") > (match_operand:DI 2 "register_operand" "r"))) > (set (match_operand:DI 0 "register_operand" "=&r") > @@ -1098,10 +1093,14 @@ > [(parallel [(set (reg:CC CC_REGNUM) > (compare:CC (match_dup 1) (match_dup 2))) > (set (match_dup 0) (minus:SI (match_dup 1) (match_dup 2)))]) > - (parallel [(set (reg:CC CC_REGNUM) > - (compare:CC (match_dup 4) (match_dup 5))) > - (set (match_dup 3) (minus:SI (minus:SI (match_dup 4) (match_dup 5)) > - (ltu:SI (reg:CC_C CC_REGNUM) (const_int 0))))])] > + (parallel [(set (reg:CC_C CC_REGNUM) > + (compare:CC_C > + (zero_extend:DI (match_dup 4)) > + (plus:DI (zero_extend:DI (match_dup 5)) > + (ltu:DI (reg:CC_C CC_REGNUM) (const_int 0))))) > + (set (match_dup 3) > + (minus:SI (minus:SI (match_dup 4) (match_dup 5)) > + (ltu:SI (reg:CC_C CC_REGNUM) (const_int 0))))])] > { > operands[3] = gen_highpart (SImode, operands[0]); > operands[0] = gen_lowpart (SImode, operands[0]); > @@ -1156,13 +1155,15 @@ > ) > > (define_insn "*subsi3_carryin_compare" > - [(set (reg:CC CC_REGNUM) > - (compare:CC (match_operand:SI 1 "s_register_operand" "r") > - (match_operand:SI 2 "s_register_operand" "r"))) > + [(set (reg:CC_C CC_REGNUM) > + (compare:CC_C > + (zero_extend:DI (match_operand:SI 1 "s_register_operand" "r")) > + (plus:DI > + (zero_extend:DI (match_operand:SI 2 "s_register_operand" "r")) > + (ltu:DI (reg:CC_C CC_REGNUM) (const_int 0))))) > (set (match_operand:SI 0 "s_register_operand" "=r") > - (minus:SI (minus:SI (match_dup 1) > - (match_dup 2)) > - (ltu:SI (reg:CC_C CC_REGNUM) (const_int 0))))] > + (minus:SI (minus:SI (match_dup 1) (match_dup 2)) > + (ltu:SI (reg:CC_C CC_REGNUM) (const_int 0))))] > "TARGET_32BIT" > "sbcs\\t%0, %1, %2" > [(set_attr "conds" "set") > @@ -1170,13 +1171,15 @@ > ) > > (define_insn "*subsi3_carryin_compare_const" > - [(set (reg:CC CC_REGNUM) > - (compare:CC (match_operand:SI 1 "reg_or_int_operand" "r") > - (match_operand:SI 2 "arm_not_operand" "K"))) > + [(set (reg:CC_C CC_REGNUM) > + (compare:CC_C > + (zero_extend:DI (match_operand:SI 1 "reg_or_int_operand" "r")) > + (plus:DI > + (zero_extend:DI (match_operand:SI 2 "arm_not_operand" "K")) > + (ltu:DI (reg:CC_C CC_REGNUM) (const_int 0))))) > (set (match_operand:SI 0 "s_register_operand" "=r") > - (minus:SI (plus:SI (match_dup 1) > - (match_dup 2)) > - (ltu:SI (reg:CC_C CC_REGNUM) (const_int 0))))] > + (minus:SI (plus:SI (match_dup 1) (match_dup 2)) > + (ltu:SI (reg:CC_C CC_REGNUM) (const_int 0))))] > "TARGET_32BIT" > "sbcs\\t%0, %1, #%B2" > [(set_attr "conds" "set") > @@ -4633,8 +4636,8 @@ > > > (define_insn_and_split "negdi2_compare" > - [(set (reg:CC CC_REGNUM) > - (compare:CC > + [(set (reg:CC_NCV CC_REGNUM) > + (compare:CC_NCV > (const_int 0) > (match_operand:DI 1 "register_operand" "0,r"))) > (set (match_operand:DI 0 "register_operand" "=r,&r") > @@ -4646,8 +4649,12 @@ > (compare:CC (const_int 0) (match_dup 1))) > (set (match_dup 0) (minus:SI (const_int 0) > (match_dup 1)))]) > - (parallel [(set (reg:CC CC_REGNUM) > - (compare:CC (const_int 0) (match_dup 3))) > + (parallel [(set (reg:CC_C CC_REGNUM) > + (compare:CC_C > + (const_int 0) > + (plus:DI > + (zero_extend:DI (match_dup 3)) > + (ltu:DI (reg:CC_C CC_REGNUM) (const_int 0))))) > (set (match_dup 2) > (minus:SI > (minus:SI (const_int 0) (match_dup 3)) > @@ -4705,12 +4712,14 @@ > ) > > (define_insn "*negsi2_carryin_compare" > - [(set (reg:CC CC_REGNUM) > - (compare:CC (const_int 0) > - (match_operand:SI 1 "s_register_operand" "r"))) > + [(set (reg:CC_C CC_REGNUM) > + (compare:CC_C > + (const_int 0) > + (plus:DI > + (zero_extend:DI (match_operand:SI 1 "s_register_operand" "r")) > + (ltu:DI (reg:CC_C CC_REGNUM) (const_int 0))))) > (set (match_operand:SI 0 "s_register_operand" "=r") > - (minus:SI (minus:SI (const_int 0) > - (match_dup 1)) > + (minus:SI (minus:SI (const_int 0) (match_dup 1)) > (ltu:SI (reg:CC_C CC_REGNUM) (const_int 0))))] > "TARGET_ARM" > "rscs\\t%0, %1, #0" > @@ -7359,12 +7368,15 @@ > "#" ; "cmp\\t%Q0, %Q1\;sbcs\\t%2, %R0, %R1" > "&& reload_completed" > [(set (reg:CC CC_REGNUM) > - (compare:CC (match_dup 0) (match_dup 1))) > - (parallel [(set (reg:CC CC_REGNUM) > - (compare:CC (match_dup 3) (match_dup 4))) > - (set (match_dup 2) > - (minus:SI (match_dup 5) > - (ltu:SI (reg:CC_C CC_REGNUM) (const_int 0))))])] > + (compare:CC (match_dup 0) (match_dup 1))) > + (parallel [(set (reg:CC_C CC_REGNUM) > + (compare:CC_C > + (zero_extend:DI (match_dup 3)) > + (plus:DI (zero_extend:DI (match_dup 4)) > + (ltu:DI (reg:CC_C CC_REGNUM) (const_int 0))))) > + (set (match_dup 2) > + (minus:SI (match_dup 5) > + (ltu:SI (reg:CC_C CC_REGNUM) (const_int 0))))])] > { > operands[3] = gen_highpart (SImode, operands[0]); > operands[0] = gen_lowpart (SImode, operands[0]); >