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 D738A3858C2D for ; Mon, 29 Jan 2024 16:03:11 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org D738A3858C2D Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=foss.arm.com Authentication-Results: sourceware.org; spf=none smtp.mailfrom=foss.arm.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org D738A3858C2D 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=1706544193; cv=none; b=OrYb2+mwx1UuMcXn+SVoAngkvX7pONT+0rQYivfnOOQYEKYo3g0xu7LcRfek4SX6v8Ew9wB7D5xT6r7dRy/pQqHhGDrONYKavjgx2uBCxiDl98+YxW+IpqueP7AWVwtJf2OpO9DX+kptM+BuIdYOny4z70zAnUD6Le0RxgQlNTY= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1706544193; c=relaxed/simple; bh=3iOJ7rMJQYjl62dtMZ/nSDyL34/f99Q6VAPsovfDd/s=; h=Message-ID:Date:MIME-Version:Subject:To:From; b=miEIzeKW5TBfPjMGIEILyPxOSVPRtMAFL58Nway2T+NFZ7RSwyAySuBtBqMos9Ge1+fvwn7qh/uUabPdqca/I+zFX5KXgQhc7vR4c4vngIocZY7a4pEBetGd+fN+RyQSwtwADNeHIg081654nK1StPTpfgDvE6gzOJsKi0Rlf0A= 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 4D74DDA7; Mon, 29 Jan 2024 08:03:55 -0800 (PST) Received: from [10.57.7.70] (unknown [10.57.7.70]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 81A0B3F738; Mon, 29 Jan 2024 08:03:10 -0800 (PST) Message-ID: <55458f8f-a81f-48f0-911b-9010f5e16333@foss.arm.com> Date: Mon, 29 Jan 2024 16:03:09 +0000 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH][GCC][Arm] Add pattern for bswap + rotate -> rev16 [Bug 108933] Content-Language: en-GB To: Matthieu Longo , "Richard Earnshaw (lists)" , gcc-patches@gcc.gnu.org Cc: Kyrylo Tkachov References: <6c8c1ae8-78b3-4310-8c75-f452e774eb3b@arm.com> <7132896e-b5ce-4442-815e-97c1a6ae0913@arm.com> From: Richard Earnshaw In-Reply-To: <7132896e-b5ce-4442-815e-97c1a6ae0913@arm.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-3487.4 required=5.0 tests=BAYES_00,BODY_8BITS,KAM_DMARC_STATUS,KAM_LAZY_DOMAIN_SECURITY,KAM_SHORT,SPF_HELO_NONE,SPF_NONE,TXREP,T_SCC_BODY_TEXT_LINE autolearn=no 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 29/01/2024 14:14, Matthieu Longo wrote: > Hi Richard, > > Please find below the new patch where I addressed your comments and > updated the changelog. > > rev16 pattern was not recognised anymore as a change in the bswap tree > pass was introducing a new GIMPLE form, not recognized by the assembly > final transformation pass. > > More details in https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108933 > > gcc/ChangeLog: > >     PR target/108933 >     * config/arm/arm.md (arm_rev16si2): Convert to define_insn. >     Correct generated RTL. >     (arm_rev16si2_alt1): Correctly handle conditional execution. >     (arm_rev16si2_alt2): Likewise. > > gcc/testsuite/ChangeLog: > >     PR target/108933 >     * gcc.target/arm/rev16.c: Moved to... >     * gcc.target/arm/rev16_1.c: ...here. >     * gcc.target/arm/rev16_2.c: New test to check that rev16 is >     emitted. Thanks. I've tweaked the commit message very slightly and pushed this. Could you please prepare backports for gcc-11 thru 13? It should just be a matter of cherry-picking the commit. R. > > On 2024-01-22 16:25, Richard Earnshaw (lists) wrote: >> On 22/01/2024 12:18, Matthieu Longo wrote: >>> rev16 pattern was not recognised anymore as a change in the bswap tree >>> pass was introducing a new GIMPLE form, not recognized by the assembly >>> final transformation pass. >>> >>> More details in https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108933 >>> >>> gcc/ChangeLog: >>> >>>          PR target/108933 >>>          * config/arm/arm.md (*arm_rev16si2_alt3): new pattern to >>> convert >>>            a bswap + rotate by 16 bits into rev16 >> >> ChangeLog entries need to be written as sentences, so start with a >> capital letter and end with a full stop; continuation lines should >> start in column 8 (one hard tab, don't use spaces).  But in this case, >> "New pattern." is sufficient. >> >>> >>> gcc/testsuite/ChangeLog: >>> >>>          PR target/108933 >>>          * gcc.target/arm/rev16.c: Moved to... >>>          * gcc.target/arm/rev16_1.c: ...here. >>>          * gcc.target/arm/rev16_2.c: New test to check that rev16 is >>>            emitted. >> >> >> +;; Similar pattern to match (rotate (bswap) 16) >> +(define_insn "*arm_rev16si2_alt3" >> +  [(set (match_operand:SI 0 "register_operand" "=l,r") >> +        (rotate:SI (bswap:SI (match_operand:SI 1 "register_operand" >> "l,r")) >> +                     (const_int 16)))] >> +  "arm_arch6" >> +  "rev16\\t%0, %1" >> +  [(set_attr "arch" "t,32") >> +   (set_attr "length" "2,4") >> +   (set_attr "type" "rev")] >> +) >> + >> >> Unfortunately, this is insufficient.  When generating Arm or Thumb2 >> code (but not thumb1) we also have to handle conditional execution: we >> need to have '%?' in the output template at the point where a >> condition code might be needed.  That means we need separate output >> templates for all three alternatives (as we need a 16-bit variant for >> thumb2 that's conditional and a 16-bit for thumb1 that isn't).  See >> the output of arm_rev16 for a guide of what is really needed. >> >> I note that the arm_rev16si2_alt1, and arm_rev16si2_alt2 patterns are >> incorrect in this regard as well; that will need fixing. >> >> I also see that arm_rev16si2 currently expands to the alt1 variant >> above; given that the preferred canonical form would now appear to use >> bswap + rotate, we should change that as well.  In fact, we can merge >> your new pattern with the expand entirely and eliminate the need to >> call gen_arm_rev16si2_alt1.  Something like: >> >> (define_insn "arm_rev16si2" >>    [(set (match_operand:SI 0 "s_register_operand") >>          (rotate:SI (bswap:SI (match_operand:SI 1 >> "s_register_operand")) (const_int 16))] >>    "arm_arch6" >>    "@ >>    rev16... >>    ... >> >> >> R. >>