From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 31661 invoked by alias); 13 Jan 2017 16:10:19 -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 31646 invoked by uid 89); 13 Jan 2017 16:10:18 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.8 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.2 spammy=Hauthentication-results:message, Hauthentication-results:header.d, H*MI:sk:3f5e553, Hauthentication-results:signed X-HELO: COL004-OMC4S11.hotmail.com Received: from col004-omc4s11.hotmail.com (HELO COL004-OMC4S11.hotmail.com) (65.55.34.213) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 13 Jan 2017 16:10:16 +0000 Received: from EUR01-VE1-obe.outbound.protection.outlook.com ([65.55.34.201]) by COL004-OMC4S11.hotmail.com over TLS secured channel with Microsoft SMTPSVC(7.5.7601.23008); Fri, 13 Jan 2017 08:10:15 -0800 Received: from DB5EUR01FT042.eop-EUR01.prod.protection.outlook.com (10.152.4.57) by DB5EUR01HT091.eop-EUR01.prod.protection.outlook.com (10.152.5.220) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384_P384) id 15.1.803.8; Fri, 13 Jan 2017 16:10:12 +0000 Received: from HE1PR0701MB2169.eurprd07.prod.outlook.com (10.152.4.59) by DB5EUR01FT042.mail.protection.outlook.com (10.152.5.100) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384_P384) id 15.1.803.8 via Frontend Transport; Fri, 13 Jan 2017 16:10:12 +0000 Received: from HE1PR0701MB2169.eurprd07.prod.outlook.com ([10.168.36.18]) by HE1PR0701MB2169.eurprd07.prod.outlook.com ([10.168.36.18]) with mapi id 15.01.0845.010; Fri, 13 Jan 2017 16:10:11 +0000 From: Bernd Edlinger To: "Richard Earnshaw (lists)" , "gcc-patches@gcc.gnu.org" CC: Ramana Radhakrishnan , Kyrill Tkachov , Wilco Dijkstra Subject: Re: [PATCH, ARM] correctly encode the CC reg data flow Date: Fri, 13 Jan 2017 16:10:00 -0000 Message-ID: References: <3f5e5538-5dd3-b416-904f-b87f115336fe@arm.com> In-Reply-To: <3f5e5538-5dd3-b416-904f-b87f115336fe@arm.com> authentication-results: arm.com; dkim=none (message not signed) header.d=none;arm.com; dmarc=none action=none header.from=hotmail.de; x-incomingtopheadermarker: OriginalChecksum:C7A099D234416CB011A18FDEE8856CD4A05FA3FF42E0E8D003B1EBFC8E556157;UpperCasedChecksum:B23E7B63479F01349FC82DBB6B976A9CBB303791CD84363299F172123FBF3515;SizeAsReceived:7785;Count:37 x-ms-exchange-messagesentrepresentingtype: 1 x-incomingheadercount: 37 x-eopattributedmessage: 0 x-microsoft-exchange-diagnostics: 1;DB5EUR01HT091;7:Po5gB/77iI3qiuwIUfKB6c5DlSbFRuSnsHLCfTYKxNhupUkiSp0uURMe1yuiWAfeHK1g80JLoqSTglmmbfe5vwrlXS0csJ+ApntAaTgHKUzfRTKJysFCwhtsQkcGYklNse0JibyWbjPv4FE8l5nb0RZJp5uas0eF8drc1b+tV2VxWxE862tRShcQuA9C50IeZSUdI0UONjJlGkW7sVH+fOKzbJ6hwxNJ0NWawnvsqUTck1A9X51jJz+hKf53aoLIPZYR4EBELbQ3zPlQTZNQQwzzPJhsiGqAoFn45DsEIBxyfoAiUmFIlM5Ry8JPNFLqmV2n302P55+ldQ9qI5Py07LPA43z+RVWuQ64wtdWUI7CdEL7JI0490KzsQHiuZ6w8SWKCRDBq7ZlPIkyG1qBB80iGBgK/ctdK3TjdS1wAPTmnSjPEQ7366bOHeXfMqTvnHNwHiaUKB+F8EV7MG4HlQ== x-forefront-antispam-report: EFV:NLI;SFV:NSPM;SFS:(10019020)(98900003);DIR:OUT;SFP:1102;SCL:1;SRVR:DB5EUR01HT091;H:HE1PR0701MB2169.eurprd07.prod.outlook.com;FPR:;SPF:None;LANG:en; x-ms-office365-filtering-correlation-id: 21f0c90c-d4a6-468a-a4aa-08d43bcea5fd x-microsoft-antispam: UriScan:;BCL:0;PCL:0;RULEID:(22001)(1601124038)(5061506344)(1603103113)(1601125047)(1701031023);SRVR:DB5EUR01HT091; x-exchange-antispam-report-cfa-test: BCL:0;PCL:0;RULEID:(444111334)(444112120)(432015012)(82015046);SRVR:DB5EUR01HT091;BCL:0;PCL:0;RULEID:;SRVR:DB5EUR01HT091; x-forefront-prvs: 018632C080 spamdiagnosticoutput: 1:99 spamdiagnosticmetadata: NSPM Content-Type: text/plain; charset="Windows-1252" Content-ID: <1D436FD721572D46BD185F21AFD95F34@eurprd07.prod.outlook.com> Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-OriginatorOrg: outlook.com X-MS-Exchange-CrossTenant-originalarrivaltime: 13 Jan 2017 16:10:11.5990 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Internet X-MS-Exchange-CrossTenant-id: 84df9e7f-e9f6-40af-b435-aaaaaaaaaaaa X-MS-Exchange-Transport-CrossTenantHeadersStamped: DB5EUR01HT091 X-SW-Source: 2017-01/txt/msg00962.txt.bz2 On 01/13/17 14:50, Richard Earnshaw (lists) wrote: > 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" "=3D&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). > Yes, this is still a bit awkward... The N and V bit will be the correct result for the subdi3_compare1 a 64-bit comparison, but zero_extend:DI (match_dup 4) (plus:DI ...) only gets the C bit correct, the expression for N and V is a different one. It probably works, because the subsi3_carryin_compare instruction sets more CC bits than the pattern does explicitly specify the value. We know the subsi3_carryin_compare also computes the NV bits, but it is hard to write down the correct rtl expression for it. In theory the pattern should describe everything correctly, maybe, like: 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 (reg:CC_NV CC_REGNUM) (compare:CC_NV (match_dup 4)) (plus:SI (match_dup 5) (ltu:SI (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))))) But I doubt that will work to set CC_REGNUM with two different modes in parallel? Another idea would be to invent a CC_CNV_NOOV mode, that implicitly defines C from the DImode result, and NV from the SImode result, similar to the CC_NOOVmode, that also leaves something open what bits it really defines? What do you think? Thanks Bernd.