diff mbox series

[RISC-V,PR,target/123883] Inefficient bit manipulation code on RISC-V port

Message ID PAVPR08MB9554BCC2DE6F7C00A89989D7BEF02@PAVPR08MB9554.eurprd08.prod.outlook.com
State Superseded
Delegated to: Jeff Law
Headers
Series [RISC-V,PR,target/123883] Inefficient bit manipulation code on RISC-V port |

Checks

Context Check Description
rivoscibot/toolchain-ci-rivos-lint success Lint passed
rivoscibot/toolchain-ci-rivos-apply-patch success Patch applied
rivoscibot/toolchain-ci-rivos-build--newlib-rv64gcv-lp64d-multilib success Build passed
rivoscibot/toolchain-ci-rivos-build--linux-rv64gcv-lp64d-multilib success Build passed
rivoscibot/toolchain-ci-rivos-build--linux-rv64gc_zba_zbb_zbc_zbs-lp64d-multilib success Build passed
rivoscibot/toolchain-ci-rivos-test fail Testing failed
linaro-tcwg-bot/tcwg_simplebootstrap_build--master-arm-bootstrap fail Build failed
linaro-tcwg-bot/tcwg_gcc_build--master-arm pending Patch applied
linaro-tcwg-bot/tcwg_gcc_build--master-aarch64 success Build passed
linaro-tcwg-bot/tcwg_simplebootstrap_build--master-aarch64-bootstrap fail Build failed

Commit Message

Dusan Stojkovic July 7, 2026, 5:37 p.m. UTC 
  This commit expands on the work by Jeff and Milan regarding generating
more Zbs instructions for RISC-V (PR123884).

For the testcase:
void foo(unsigned char *data, unsigned int lo_bit) {
  unsigned int mask = ((1UL << 1) - 1) << lo_bit;
  *data = (*data & ~mask) | ((1 << lo_bit) & mask)
}

Without their patch the following assembly would be produced:
foo(unsigned char*, unsigned int):
    lbu     a4,0(a0)
    li      a5,1
    sllw    a5,a5,a1
     xor     a5,a4,a5
     bset    a1,x0,a1
     and     a5,a5,a1
     xor     a4,a4,a5
     sb      a4,0(a0)
     ret

With this patch it would be transformed into:
foo(unsigned char*, unsigned int):
    lbu   a5,0(a0)
    li    a4,1
    sllw  a4,a4,a1
    bclr  a5,a5,a1
    or    a5,a5,a4
    sb    a5,0(a0)
    ret

Thus, in the combine pass arises a new pattern to try:
Trying 17, 18 -> 22:
   17: r155:SI=0x1
   18: r156:DI=sign_extend(r155:SI<<r143:DI#0)
      REG_DEAD r155:SI
      REG_DEAD r143:DI
      REG_EQUAL sign_extend(0x1<<r143:DI#0)
   22: r159:DI=r152:DI|r156:DI
      REG_DEAD r156:DI
      REG_DEAD r152:DI
Failed to match this instruction:
(set (reg:DI 159)
    (ior:DI (sign_extend:DI (ashift:SI (const_int 1 [0x1])
                (subreg:QI (reg/v:DI 143 [ lo_bitD.2469 ]) 0)))
        (reg:DI 152)))
Splitting with gen_split_161 (bitmanip.md:796)  <- pattern added in PR123884
Successfully matched this instruction:
(set (reg:DI 159)
    (ior:DI (ashift:DI (const_int 1 [0x1])
            (subreg:QI (reg/v:DI 143 [ lo_bitD.2469 ]) 0))
        (reg:DI 152)))
Successfully matched this instruction:
(set (reg:DI 159)
    (sign_extend:DI (subreg:SI (reg:DI 159) 0)))

And ultimately generates the following assembly:
foo:
        lbu     a5,0(a0)
        bset    a5,a5,a1
        sb      a5,0(a0)
        ret

The match.pd pattern is inspired by Jeff's notes in the bugreport.

Not all testcases added by this patch simplify completely.
*For rv32 the smaller type to ULL produces poor assembly still
*For rv64 the UI to UL/ULL are still not clean.

Regression tested on x86, rv64 and rv32.

 2026-07-07  Dusan Stojkovic  <Dusan.Stojkovic@rt-rk.com>

      PR target/123883

gcc/ChangeLog:

      * match.pd: New pattern.

gcc/testsuite/ChangeLog:

      * gcc.target/riscv/pr123883.c: New test.

Co-authored-by: Jeff Law <jeffrey.law@oss.qualcomm.com>




CONFIDENTIALITY: The contents of this e-mail are confidential and intended only for the above addressee(s). If you are not the intended recipient, or the person responsible for delivering it to the intended recipient, copying or delivering it to anyone else or using it in any unauthorized manner is prohibited and may be unlawful. If you receive this e-mail by mistake, please notify the sender and the systems administrator at straymail@rt-rk.com immediately.
---
 gcc/match.pd                              | 22 ++++++++++
 gcc/testsuite/gcc.target/riscv/pr123883.c | 49 +++++++++++++++++++++++
 2 files changed, 71 insertions(+)
 create mode 100644 gcc/testsuite/gcc.target/riscv/pr123883.c

Comments

Andrea Pinski July 8, 2026, 6:14 a.m. UTC | #1 
On Tue, Jul 7, 2026 at 10:38 AM Dusan Stojkovic
<Dusan.Stojkovic@rt-rk.com> wrote:
>
> This commit expands on the work by Jeff and Milan regarding generating
> more Zbs instructions for RISC-V (PR123884).
>
> For the testcase:
> void foo(unsigned char *data, unsigned int lo_bit) {
>   unsigned int mask = ((1UL << 1) - 1) << lo_bit;
>   *data = (*data & ~mask) | ((1 << lo_bit) & mask)
> }
>
> Without their patch the following assembly would be produced:
> foo(unsigned char*, unsigned int):
>     lbu     a4,0(a0)
>     li      a5,1
>     sllw    a5,a5,a1
>      xor     a5,a4,a5
>      bset    a1,x0,a1
>      and     a5,a5,a1
>      xor     a4,a4,a5
>      sb      a4,0(a0)
>      ret
>
> With this patch it would be transformed into:
> foo(unsigned char*, unsigned int):
>     lbu   a5,0(a0)
>     li    a4,1
>     sllw  a4,a4,a1
>     bclr  a5,a5,a1
>     or    a5,a5,a4
>     sb    a5,0(a0)
>     ret
>
> Thus, in the combine pass arises a new pattern to try:
> Trying 17, 18 -> 22:
>    17: r155:SI=0x1
>    18: r156:DI=sign_extend(r155:SI<<r143:DI#0)
>       REG_DEAD r155:SI
>       REG_DEAD r143:DI
>       REG_EQUAL sign_extend(0x1<<r143:DI#0)
>    22: r159:DI=r152:DI|r156:DI
>       REG_DEAD r156:DI
>       REG_DEAD r152:DI
> Failed to match this instruction:
> (set (reg:DI 159)
>     (ior:DI (sign_extend:DI (ashift:SI (const_int 1 [0x1])
>                 (subreg:QI (reg/v:DI 143 [ lo_bitD.2469 ]) 0)))
>         (reg:DI 152)))
> Splitting with gen_split_161 (bitmanip.md:796)  <- pattern added in PR123884
> Successfully matched this instruction:
> (set (reg:DI 159)
>     (ior:DI (ashift:DI (const_int 1 [0x1])
>             (subreg:QI (reg/v:DI 143 [ lo_bitD.2469 ]) 0))
>         (reg:DI 152)))
> Successfully matched this instruction:
> (set (reg:DI 159)
>     (sign_extend:DI (subreg:SI (reg:DI 159) 0)))
>
> And ultimately generates the following assembly:
> foo:
>         lbu     a5,0(a0)
>         bset    a5,a5,a1
>         sb      a5,0(a0)
>         ret
>
> The match.pd pattern is inspired by Jeff's notes in the bugreport.
>
> Not all testcases added by this patch simplify completely.
> *For rv32 the smaller type to ULL produces poor assembly still
> *For rv64 the UI to UL/ULL are still not clean.
>
> Regression tested on x86, rv64 and rv32.
>
>  2026-07-07  Dusan Stojkovic  <Dusan.Stojkovic@rt-rk.com>
>
>       PR target/123883
>
> gcc/ChangeLog:
>
>       * match.pd: New pattern.
>
> gcc/testsuite/ChangeLog:
>
>       * gcc.target/riscv/pr123883.c: New test.
>
> Co-authored-by: Jeff Law <jeffrey.law@oss.qualcomm.com>

Some fixes:
+/* (T)(1 << x) & (T)(1 << x) -> (T)(1 << x),
+ *  but keep the shift in the wider type to avoid introducing
+ *  undefined behaviour.  */
+(simplify
+ (bit_and:c
+  (convert? (lshift@2 integer_onep@1 @0))
+  (convert? (lshift@3 integer_onep@4 @0)))
+ (with
+  {
+    tree ltype0 = TREE_TYPE (@2);
+    tree ltype1 = TREE_TYPE (@3);
+    tree largertype = ltype0;
+    tree largertypeone = build_one_cst (largertype);
Remove largertypeone.
+  }
+  (if (INTEGRAL_TYPE_P (type)
+       && INTEGRAL_TYPE_P (ltype0)
+       && INTEGRAL_TYPE_P (ltype1)
+       && element_precision (type) <= element_precision (ltype0)
+       && element_precision (type) <= element_precision (ltype1))

So the check should be:
 && element_precision (type) <= element_precision (ltype1)
 && element_precision (ltype1) <= element_precision (ltype0))

Such that  `type <= itype1 <= itype0`

+   (convert:type
+    (lshift:largertype { largertypeone; } @0)))))

Replace `{ largertypeone; }` with `@1`.
Even though INTEGER_CSTs are shared and small integers are kept
around; this makes the code slightly faster as you don't need to look
up the constant.

if you want a future expansion on this, a nice change would remove the
`:c` and add a check for the other way around; that is  `type <=
itype0 <= itype1` and using itype1 for the inner type.

Thanks,
Andrea



>
>
>
>
> CONFIDENTIALITY: The contents of this e-mail are confidential and intended only for the above addressee(s). If you are not the intended recipient, or the person responsible for delivering it to the intended recipient, copying or delivering it to anyone else or using it in any unauthorized manner is prohibited and may be unlawful. If you receive this e-mail by mistake, please notify the sender and the systems administrator at straymail@rt-rk.com immediately.
diff mbox series

Patch

diff --git a/gcc/match.pd b/gcc/match.pd
index a7cec25dbad..b0c13e72d97 100644
--- a/gcc/match.pd
+++ b/gcc/match.pd
@@ -12335,6 +12335,28 @@  and,
         && @0 == @3)
     (bit_xor (rrotate @0 @4) @2)))
 
+/* (T)(1 << x) & (T)(1 << x) -> (T)(1 << x),
+ *  but keep the shift in the wider type to avoid introducing
+ *  undefined behaviour.  */
+(simplify
+ (bit_and:c
+  (convert? (lshift@2 integer_onep@1 @0))
+  (convert? (lshift@3 integer_onep@4 @0)))
+ (with
+  {
+    tree ltype0 = TREE_TYPE (@2);
+    tree ltype1 = TREE_TYPE (@3);
+    tree largertype = ltype0;
+    tree largertypeone = build_one_cst (largertype);
+  }
+  (if (INTEGRAL_TYPE_P (type)
+       && INTEGRAL_TYPE_P (ltype0)
+       && INTEGRAL_TYPE_P (ltype1)
+       && element_precision (type) <= element_precision (ltype0)
+       && element_precision (type) <= element_precision (ltype1))
+   (convert:type
+    (lshift:largertype { largertypeone; } @0)))))
+
 /* Optimize extraction from a uniform vector to a representative element as
    long as the requested element is within range.  */
 (simplify (IFN_VEC_EXTRACT (vec_duplicate @0) INTEGER_CST@1)
diff --git a/gcc/testsuite/gcc.target/riscv/pr123883.c b/gcc/testsuite/gcc.target/riscv/pr123883.c
new file mode 100644
index 00000000000..e4f86da0650
--- /dev/null
+++ b/gcc/testsuite/gcc.target/riscv/pr123883.c
@@ -0,0 +1,49 @@ 
+/* { dg-do compile } */
+/* { dg-options "-march=rv64gcb -mabi=lp64d" { target rv64 } } */
+/* { dg-options "-march=rv32gcb -mabi=ilp32" { target rv32 } } */
+/* { dg-skip-if "" { *-*-* } { "-O0" "-O1" "-Og" "-Os" "-Oz" } } */
+
+#define TEST_STORE_NARROW_SHIFT(TYPE, WTYPE, NAME)               \
+void                                                             \
+NAME (TYPE *data, unsigned int lo_bit)                           \
+{                                                                \
+  WTYPE mask = (((WTYPE) 1UL << 1) - 1) << lo_bit;               \
+  *data = (*data & ~mask) | ((1U << lo_bit) & mask);             \
+}
+
+#define TEST_RET_NARROW_SHIFT(TYPE, WTYPE, NAME)                 \
+TYPE                                                             \
+NAME (TYPE data, unsigned int lo_bit)                            \
+{                                                                \
+  WTYPE mask = (((WTYPE) 1 << 1) - 1) << lo_bit;                 \
+  return (data & ~mask) | ((1U << lo_bit) & mask);               \
+}
+
+TEST_STORE_NARROW_SHIFT(unsigned char,  unsigned long, store_uc_ul_narrow)
+TEST_STORE_NARROW_SHIFT(unsigned short, unsigned long, store_us_ul_narrow)
+TEST_STORE_NARROW_SHIFT(unsigned int,   unsigned long, store_ui_ul_narrow)
+
+TEST_RET_NARROW_SHIFT(unsigned char,    unsigned long, ret_uc_ul_narrow)
+TEST_RET_NARROW_SHIFT(unsigned short,   unsigned long, ret_us_ul_narrow)
+TEST_RET_NARROW_SHIFT(unsigned int,     unsigned long, ret_ui_ul_narrow)
+
+TEST_STORE_NARROW_SHIFT(unsigned char,  unsigned long long, store_uc_ull_narrow)
+TEST_STORE_NARROW_SHIFT(unsigned short, unsigned long long, store_us_ull_narrow)
+TEST_STORE_NARROW_SHIFT(unsigned int,   unsigned long long, store_ui_ull_narrow)
+
+TEST_RET_NARROW_SHIFT(unsigned char,    unsigned long long, ret_uc_ull_narrow)
+TEST_RET_NARROW_SHIFT(unsigned short,   unsigned long long, ret_us_ull_narrow)
+TEST_RET_NARROW_SHIFT(unsigned int,     unsigned long long, ret_ui_ull_narrow)
+
+
+/* { dg-final { scan-assembler-times "bset" 12 { target rv64 } } } */
+/* Current remaining missed SI-mode cases.  */
+/* { dg-final { scan-assembler-times "sllw" 2 { target rv64 } } } */
+/* { dg-final { scan-assembler-times "binv" 2 { target rv64 } } } */
+/* { dg-final { scan-assembler-times "sext.w" 4 { target rv64 } } } */
+
+/* Current remaining ULL wide-mask cases on RV32.  */
+/* { dg-final { scan-assembler-times "bset" 14 { target rv32 } } } */
+/* { dg-final { scan-assembler-times "binv" 2 { target rv32 } } } */
+/* { dg-final { scan-assembler-times "srai" 4 { target rv32 } } } */
+