diff mbox series

RISC-V: avoid splitting small constant in <or_optab>i<mode>_extrabit pattern

Message ID 20230410050701.10254-1-mynameisxiaou@gmail.com
State Accepted
Headers
Series RISC-V: avoid splitting small constant in <or_optab>i<mode>_extrabit pattern |

Checks

Context Check Description
snail/gcc-patch-check success Github commit url

Commit Message

Lin Sinan April 10, 2023, 5:07 a.m. UTC 
  From: Sinan Lin <linsinan.lsn@linux.alibaba.com>

there is no need to split an xori/ori with an small constant. take the test
case `int foo(int idx) { return idx|3; }` as an example,

rv64im_zba generates:
        ori     a0,a0,3
        ret
but, rv64im_zba_zbs generates:
        ori     a0,a0,1
        ori     a0,a0,2
        ret

with this change, insn `ori r2,r1,3` will not be splitted in zbs.
---
 gcc/config/riscv/predicates.md                     |  2 +-
 .../gcc.target/riscv/zbs-extra-bit-or-twobits.c    | 14 ++++++++++++++
 2 files changed, 15 insertions(+), 1 deletion(-)
 create mode 100644 gcc/testsuite/gcc.target/riscv/zbs-extra-bit-or-twobits.c

Comments

Jeff Law April 10, 2023, 3:57 p.m. UTC | #1 
On 4/9/23 23:07, Lin Sinan via Gcc-patches wrote:
> From: Sinan Lin <linsinan.lsn@linux.alibaba.com>
> 
> there is no need to split an xori/ori with an small constant. take the test
> case `int foo(int idx) { return idx|3; }` as an example,
> 
> rv64im_zba generates:
>          ori     a0,a0,3
>          ret
> but, rv64im_zba_zbs generates:
>          ori     a0,a0,1
>          ori     a0,a0,2
>          ret
> 
> with this change, insn `ori r2,r1,3` will not be splitted in zbs.
> ---
>   gcc/config/riscv/predicates.md                     |  2 +-
>   .../gcc.target/riscv/zbs-extra-bit-or-twobits.c    | 14 ++++++++++++++
>   2 files changed, 15 insertions(+), 1 deletion(-)
>   create mode 100644 gcc/testsuite/gcc.target/riscv/zbs-extra-bit-or-twobits.c
A minor oversight in the VRULL patches in this space.  This is actually 
a regression as we were previously generating the single [xo]ri.


The patch looks fine, though it does need to go through a test cycle.

jeff
Philipp Tomsich April 10, 2023, 8:59 p.m. UTC | #2 
On Mon, 10 Apr 2023 at 17:57, Jeff Law <jeffreyalaw@gmail.com> wrote:
>
>
>
> On 4/9/23 23:07, Lin Sinan via Gcc-patches wrote:
> > From: Sinan Lin <linsinan.lsn@linux.alibaba.com>
> >
> > there is no need to split an xori/ori with an small constant. take the test
> > case `int foo(int idx) { return idx|3; }` as an example,
> >
> > rv64im_zba generates:
> >          ori     a0,a0,3
> >          ret
> > but, rv64im_zba_zbs generates:
> >          ori     a0,a0,1
> >          ori     a0,a0,2
> >          ret
> >
> > with this change, insn `ori r2,r1,3` will not be splitted in zbs.
> > ---
> >   gcc/config/riscv/predicates.md                     |  2 +-
> >   .../gcc.target/riscv/zbs-extra-bit-or-twobits.c    | 14 ++++++++++++++
> >   2 files changed, 15 insertions(+), 1 deletion(-)
> >   create mode 100644 gcc/testsuite/gcc.target/riscv/zbs-extra-bit-or-twobits.c
> A minor oversight in the VRULL patches in this space.  This is actually
> a regression as we were previously generating the single [xo]ri.

Thanks for catching this one!

I looked this change over and it looks fine.  I hope this is the last
fallout from this set of changes.

>
> The patch looks fine, though it does need to go through a test cycle.
>
> jeff
>
Jeff Law April 11, 2023, 4:19 p.m. UTC | #3 
On 4/10/23 14:59, Philipp Tomsich wrote:
> On Mon, 10 Apr 2023 at 17:57, Jeff Law <jeffreyalaw@gmail.com> wrote:
>>
>>
>>
>> On 4/9/23 23:07, Lin Sinan via Gcc-patches wrote:
>>> From: Sinan Lin <linsinan.lsn@linux.alibaba.com>
>>>
>>> there is no need to split an xori/ori with an small constant. take the test
>>> case `int foo(int idx) { return idx|3; }` as an example,
>>>
>>> rv64im_zba generates:
>>>           ori     a0,a0,3
>>>           ret
>>> but, rv64im_zba_zbs generates:
>>>           ori     a0,a0,1
>>>           ori     a0,a0,2
>>>           ret
>>>
>>> with this change, insn `ori r2,r1,3` will not be splitted in zbs.
>>> ---
>>>    gcc/config/riscv/predicates.md                     |  2 +-
>>>    .../gcc.target/riscv/zbs-extra-bit-or-twobits.c    | 14 ++++++++++++++
>>>    2 files changed, 15 insertions(+), 1 deletion(-)
>>>    create mode 100644 gcc/testsuite/gcc.target/riscv/zbs-extra-bit-or-twobits.c
>> A minor oversight in the VRULL patches in this space.  This is actually
>> a regression as we were previously generating the single [xo]ri.
> 
> Thanks for catching this one!
> 
> I looked this change over and it looks fine.  I hope this is the last
> fallout from this set of changes.
Just for completeness, I bootstrapped and regression tested Sinan's 
change on rv64.  Given it's a regression, I'm going to go ahead and 
commit it to the trunk momentarily.

jeff
Jeff Law April 11, 2023, 4:28 p.m. UTC | #4 
On 4/9/23 23:07, Lin Sinan via Gcc-patches wrote:
> From: Sinan Lin <linsinan.lsn@linux.alibaba.com>
> 
> there is no need to split an xori/ori with an small constant. take the test
> case `int foo(int idx) { return idx|3; }` as an example,
> 
> rv64im_zba generates:
>          ori     a0,a0,3
>          ret
> but, rv64im_zba_zbs generates:
>          ori     a0,a0,1
>          ori     a0,a0,2
>          ret
> 
> with this change, insn `ori r2,r1,3` will not be splitted in zbs.
As noted, this was actually a regression.  I've bootstrapped this change 
on rv64 with no regressions and pushed it to the trunk.

Thanks!

jeff
diff mbox series

Patch

diff --git a/gcc/config/riscv/predicates.md b/gcc/config/riscv/predicates.md
index 0d9d7701c7e..8654dbc5943 100644
--- a/gcc/config/riscv/predicates.md
+++ b/gcc/config/riscv/predicates.md
@@ -399,7 +399,7 @@ 
 (define_predicate "uimm_extra_bit_or_twobits"
   (and (match_code "const_int")
        (ior (match_operand 0 "uimm_extra_bit_operand")
-	    (match_operand 0 "const_twobits_operand"))))
+	    (match_operand 0 "const_twobits_not_arith_operand"))))
 
 ;; A CONST_INT operand that fits into the negative half of a
 ;; signed-immediate after a single cleared top bit has been
diff --git a/gcc/testsuite/gcc.target/riscv/zbs-extra-bit-or-twobits.c b/gcc/testsuite/gcc.target/riscv/zbs-extra-bit-or-twobits.c
new file mode 100644
index 00000000000..ef7ed60461a
--- /dev/null
+++ b/gcc/testsuite/gcc.target/riscv/zbs-extra-bit-or-twobits.c
@@ -0,0 +1,14 @@ 
+/* { dg-do compile } */
+/* { dg-options "-march=rv64gc_zbs -mabi=lp64" } */
+/* { dg-skip-if "" { *-*-* } { "-O0" "-Og" } } */
+
+int or_two_bit(int idx) {
+    return idx|3;
+}
+
+int xor_two_bit(int idx) {
+    return idx^3;
+}
+
+/* { dg-final { scan-assembler-times "\tori\t" 1 } } */
+/* { dg-final { scan-assembler-times "\txori\t" 1 } } */