From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by sourceware.org (Postfix) with ESMTP id CDAC03858CDA for ; Wed, 21 Feb 2024 15:58:13 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org CDAC03858CDA Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=arm.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=arm.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org CDAC03858CDA Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=217.140.110.172 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1708531095; cv=none; b=XQjsC7fGndUqWV4Q7tI2k23YwBpFCCv1sEQrVOgfS9ZC8FwWHo36HTBvubREXz3uwf7vtsIJqFYWLQuA+Y76TfyrCz29t4e48K+et9GfTmunPZ7NPLYw8tHp7ZZtDjnf4T3KayZsh8JLCu1CmB3N/yQp/tQ4om+ekpYfltmxdq8= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1708531095; c=relaxed/simple; bh=8Woblvtun8Ublkzv9LYI24Faa2S+aCHGdsJ7Pgghb8E=; h=Message-ID:Date:MIME-Version:Subject:To:From; b=IIqXTR3Wgsz2fCFTiUdtwmxLr3jorjrpLgUmSmjughVCSEXvPrKMo3JBhihdNIgEwfExU/V1MdUBTFDHKWyhUvA3Uje4EvW7HBS4KtXQ8N0uT3NaJH8NlZFH8dmuZ0qVyNaPp8oyGQAjrWeMY+mAPVC/XyK2pCNnt8dGaXnbg/I= ARC-Authentication-Results: i=1; server2.sourceware.org Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 9C6A11007; Wed, 21 Feb 2024 07:58:51 -0800 (PST) Received: from [10.2.78.54] (e120077-lin.cambridge.arm.com [10.2.78.54]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 7AF913F73F; Wed, 21 Feb 2024 07:58:12 -0800 (PST) Message-ID: Date: Wed, 21 Feb 2024 15:58:11 +0000 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH] ARM: Fix conditional execution [PR113915] Content-Language: en-GB To: Wilco Dijkstra , Kyrylo Tkachov Cc: GCC Patches References: From: "Richard Earnshaw (lists)" In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-3497.6 required=5.0 tests=BAYES_00,GIT_PATCH_0,KAM_DMARC_NONE,KAM_DMARC_STATUS,KAM_LAZY_DOMAIN_SECURITY,KAM_LOTSOFHASH,KAM_SHORT,SPF_HELO_NONE,SPF_NONE,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On 21/02/2024 14:34, Wilco Dijkstra wrote: > > By default most patterns can be conditionalized on Arm targets. However > Thumb-2 predication requires the "predicable" attribute be explicitly > set to "yes". Most patterns are shared between Arm and Thumb(-2) and are > marked with "predicable". Given this sharing, it does not make sense to > use a different default for Arm. So only consider conditional execution > of instructions that have the predicable attribute set to yes. This ensures > that patterns not explicitly marked as such are never accidentally > conditionally executed like in the PR. > > GLIBC codesize was ~0.014% worse due to atomic operations now being > unconditional and a small number of patterns not setting "predicable". > > Passes regress and bootstrap, OK for commit? > > gcc/ChangeLog: > PR target/113915 > * config/arm/arm.md (NOCOND): Improve comment. > * config/arm/arm.cc (arm_final_prescan_insn): Add check for > PREDICABLE_YES. > > gcc/testsuite/ChangeLog: > PR target/113915 > * gcc.target/arm/builtin-bswap-1.c: Fix test. > > --- > > diff --git a/gcc/config/arm/arm.cc b/gcc/config/arm/arm.cc > index c44047c377a802d0c1dc1406df1b88a6b079607b..29771d284831a995adcf9adbb525396fbabb1ea2 100644 > --- a/gcc/config/arm/arm.cc > +++ b/gcc/config/arm/arm.cc > @@ -25610,11 +25610,12 @@ arm_final_prescan_insn (rtx_insn *insn) > break; > > case INSN: > - /* Instructions using or affecting the condition codes make it > - fail. */ > + /* Check the instruction is explicitly marked as predicable. > + Instructions using or affecting the condition codes are not. */ > scanbody = PATTERN (this_insn); > if (!(GET_CODE (scanbody) == SET > || GET_CODE (scanbody) == PARALLEL) > + || get_attr_predicable (this_insn) != PREDICABLE_YES > || get_attr_conds (this_insn) != CONDS_NOCOND) > fail = TRUE; > break; > diff --git a/gcc/config/arm/arm.md b/gcc/config/arm/arm.md > index 5816409f86f1106b410c5e21d77e599b485f85f2..671f093862259c2c0df93a986fc22fa56a8ea6c7 100644 > --- a/gcc/config/arm/arm.md > +++ b/gcc/config/arm/arm.md > @@ -307,6 +307,8 @@ > ; > ; NOCOND means that the instruction does not use or alter the condition > ; codes but can be converted into a conditionally exectuted instruction. > +; Given that NOCOND is the default for most instructions if omitted, > +; the attribute predicable must be set to yes as well. > > (define_attr "conds" "use,set,clob,unconditional,nocond" > (if_then_else While this is ok, > diff --git a/gcc/testsuite/gcc.target/arm/builtin-bswap-1.c b/gcc/testsuite/gcc.target/arm/builtin-bswap-1.c > index c1e7740d14d3ca4e93a71e38b12f82c19791a204..3de7cea81c1128c2fe5a9e1216e6b027d26bcab9 100644 > --- a/gcc/testsuite/gcc.target/arm/builtin-bswap-1.c > +++ b/gcc/testsuite/gcc.target/arm/builtin-bswap-1.c > @@ -5,14 +5,8 @@ > of the instructions. Add an -mtune option known to facilitate that. */ > /* { dg-additional-options "-O2 -mtune=cortex-a53" } */ > /* { dg-final { scan-assembler-not "orr\[ \t\]" } } */ > -/* { dg-final { scan-assembler-times "revsh\\t" 1 { target { arm_nothumb } } } } */ > -/* { dg-final { scan-assembler-times "revshne\\t" 1 { target { arm_nothumb } } } } */ > -/* { dg-final { scan-assembler-times "revsh\\t" 2 { target { ! arm_nothumb } } } } */ > -/* { dg-final { scan-assembler-times "rev16\\t" 1 { target { arm_nothumb } } } } */ > -/* { dg-final { scan-assembler-times "rev16ne\\t" 1 { target { arm_nothumb } } } } */ > -/* { dg-final { scan-assembler-times "rev16\\t" 2 { target { ! arm_nothumb } } } } */ > -/* { dg-final { scan-assembler-times "rev\\t" 2 { target { arm_nothumb } } } } */ > -/* { dg-final { scan-assembler-times "revne\\t" 2 { target { arm_nothumb } } } } */ > -/* { dg-final { scan-assembler-times "rev\\t" 4 { target { ! arm_nothumb } } } } */ > +/* { dg-final { scan-assembler-times "revsh\\t" 2 } } */ > +/* { dg-final { scan-assembler-times "rev16\\t" 2 } } */ > +/* { dg-final { scan-assembler-times "rev\\t" 4 } } */ > > #include "builtin-bswap.x" This bit isn't. The correct fix here is to fix the pattern(s) concerned to add the missing predicate. Note that builtin-bswap.x explicitly mentions predicated mnemonics in the comments. R.