Not sure how I missed zbb-rol-ror-03.c failing with the prior version of
this patch. But thankfully it's easily resolved.
Looking at the testcase I think the old code generation was bogus as it
was folding in an andi dst,src,63 with a rorw instruction. It would be
OK to fold with a ror and it would be ok to fold into rorw if the mask
was 31, but 63 into a rorw looks very wrong to me.
So testcase adjusted. No other changes. Pushing to the trunk. Final
patch attached.
jeff
ps. This is a good backport candidate after simmering on the trunk for
a while.
commit 05d75c5bfcf923bc0258b79a08c5861590c5a2b9
Author: Jeff Law <j...@ventanamicro.com>
Date: Mon May 5 17:14:29 2025 -0600
[RISC-V][PR target/119971] Avoid losing shift count masking
As is outlined in the PR, we have a few define_insn_and_split patterns which
optimize away explicit masking of shift/bit positions when the masking
matches
what the hardware's behavior.
A small number of those define_insn_and_split patterns generate a single
instruction. It's fairly elegant in that we were essentially just rewriting
the RTL to match an existing pattern.
In one case we'd do the rewriting and later turn a 32bit shift into a bset.
That's not safe because the masking of a 32bit shift uses 0x1f while
masking on
bset uses 0x3f on rv64. The net was incorrect code as seen in the BZ
entry.
The fix is pretty simple. There's no real reason we need to use a
define_insn_and_split. It was just convenient. Instead we can use a simple
define_insn. That avoids a change in the masking behavior for the shift
count/bit position and the masking stays in the RTL.
I quickly scanned the entire port and didn't see any additional
define_insn_and_splits that obviously generated a single instruction outside
the shift/rotate space, though in the vector space that's nontrivial to
ascertain.
This was been run through my tester for the cross configurations, but not
the
native bootstrap/regression test (yet).
PR target/119971
gcc/
* config/riscv/bitmanip.md (rotation with masked count): Rewrite
as define_insn patterns. Fix formatting.
* config/riscv/riscv.md (shift with masked count): Similarly.
gcc/testsuite
* gcc.target/riscv/pr119971.c: New test.
* gcc.target/riscv/zbb-rol-ror-03.c: Adjust test slightly.
diff --git a/gcc/config/riscv/bitmanip.md b/gcc/config/riscv/bitmanip.md
index d0919ece31f..20d03dc8792 100644
--- a/gcc/config/riscv/bitmanip.md
+++ b/gcc/config/riscv/bitmanip.md
@@ -423,39 +423,40 @@ (define_insn "rotlsi3_sext"
"rolw\t%0,%1,%2"
[(set_attr "type" "bitmanip")])
-(define_insn_and_split "*<bitmanip_optab><GPR:mode>3_mask"
- [(set (match_operand:GPR 0 "register_operand" "= r")
- (bitmanip_rotate:GPR
- (match_operand:GPR 1 "register_operand" " r")
- (match_operator 4 "subreg_lowpart_operator"
- [(and:GPR2
- (match_operand:GPR2 2 "register_operand" "r")
- (match_operand 3 "<GPR:shiftm1>" "<GPR:shiftm1p>"))])))]
+(define_insn "*<bitmanip_optab><mode>3_mask"
+ [(set (match_operand:X 0 "register_operand" "=r")
+ (bitmanip_rotate:X
+ (match_operand:X 1 "register_operand" "r")
+ (match_operator 4 "subreg_lowpart_operator"
+ [(and:X (match_operand:X 2 "register_operand" "r")
+ (match_operand 3 "<X:shiftm1>" "<X:shiftm1p>"))])))]
"TARGET_ZBB || TARGET_ZBKB"
- "#"
- "&& 1"
- [(set (match_dup 0)
- (bitmanip_rotate:GPR (match_dup 1)
- (match_dup 2)))]
- "operands[2] = gen_lowpart (QImode, operands[2]);"
+ "<bitmanip_insn>\t%0,%1,%2"
[(set_attr "type" "bitmanip")
- (set_attr "mode" "<GPR:MODE>")])
+ (set_attr "mode" "<X:MODE>")])
-(define_insn_and_split "*<bitmanip_optab>si3_sext_mask"
- [(set (match_operand:DI 0 "register_operand" "= r")
- (sign_extend:DI (bitmanip_rotate:SI
- (match_operand:SI 1 "register_operand" " r")
- (match_operator 4 "subreg_lowpart_operator"
- [(and:GPR
- (match_operand:GPR 2 "register_operand" "r")
- (match_operand 3 "const_si_mask_operand"))]))))]
+(define_insn "*<bitmanip_optab>3_mask_si"
+ [(set (match_operand:SI 0 "register_operand" "=r")
+ (bitmanip_rotate:SI
+ (match_operand:SI 1 "register_operand" "r")
+ (match_operator 3 "subreg_lowpart_operator"
+ [(and:X (match_operand:SI 2 "register_operand" "r")
+ (const_int 31))])))]
"TARGET_64BIT && (TARGET_ZBB || TARGET_ZBKB)"
- "#"
- "&& 1"
- [(set (match_dup 0)
- (sign_extend:DI (bitmanip_rotate:SI (match_dup 1)
- (match_dup 2))))]
- "operands[2] = gen_lowpart (QImode, operands[2]);"
+ "<bitmanip_insn>w\t%0,%1,%2"
+ [(set_attr "type" "bitmanip")
+ (set_attr "mode" "SI")])
+
+(define_insn "*<bitmanip_optab>si3_sext_mask"
+ [(set (match_operand:DI 0 "register_operand" "=r")
+ (sign_extend:DI
+ (bitmanip_rotate:SI
+ (match_operand:SI 1 "register_operand" "r")
+ (match_operator 3 "subreg_lowpart_operator"
+ [(and:X (match_operand:GPR 2 "register_operand" "r")
+ (const_int 31))]))))]
+ "TARGET_64BIT && (TARGET_ZBB || TARGET_ZBKB)"
+ "<bitmanip_insn>w\t%0,%1,%2"
[(set_attr "type" "bitmanip")
(set_attr "mode" "DI")])
diff --git a/gcc/config/riscv/riscv.md b/gcc/config/riscv/riscv.md
index c34eadb01c9..15c89ff4e3d 100644
--- a/gcc/config/riscv/riscv.md
+++ b/gcc/config/riscv/riscv.md
@@ -2937,7 +2937,7 @@ (define_insn "<optab>di3"
[(set_attr "type" "shift")
(set_attr "mode" "DI")])
-(define_insn_and_split "*<optab><GPR:mode>3_mask_1"
+(define_insn "*<optab><GPR:mode>3_mask_1"
[(set (match_operand:GPR 0 "register_operand" "= r")
(any_shift:GPR
(match_operand:GPR 1 "register_operand" " r")
@@ -2946,12 +2946,7 @@ (define_insn_and_split "*<optab><GPR:mode>3_mask_1"
(match_operand:GPR2 2 "register_operand" "r")
(match_operand 3 "<GPR:shiftm1>"))])))]
""
- "#"
- "&& 1"
- [(set (match_dup 0)
- (any_shift:GPR (match_dup 1)
- (match_dup 2)))]
- "operands[2] = gen_lowpart (QImode, operands[2]);"
+ "<insn>\t%0,%1,%2"
[(set_attr "type" "shift")
(set_attr "mode" "<GPR:MODE>")])
@@ -2970,7 +2965,7 @@ (define_insn "<optab>si3_extend"
[(set_attr "type" "shift")
(set_attr "mode" "SI")])
-(define_insn_and_split "*<optab>si3_extend_mask"
+(define_insn "*<optab>si3_extend_mask"
[(set (match_operand:DI 0 "register_operand" "= r")
(sign_extend:DI
(any_shift:SI
@@ -2980,13 +2975,7 @@ (define_insn_and_split "*<optab>si3_extend_mask"
(match_operand:GPR 2 "register_operand" " r")
(match_operand 3 "const_si_mask_operand"))]))))]
"TARGET_64BIT"
- "#"
- "&& 1"
- [(set (match_dup 0)
- (sign_extend:DI
- (any_shift:SI (match_dup 1)
- (match_dup 2))))]
- "operands[2] = gen_lowpart (QImode, operands[2]);"
+ "<insn>w\t%0,%1,%2"
[(set_attr "type" "shift")
(set_attr "mode" "SI")])
diff --git a/gcc/testsuite/gcc.target/riscv/pr119971.c
b/gcc/testsuite/gcc.target/riscv/pr119971.c
new file mode 100644
index 00000000000..c3f23b05ec3
--- /dev/null
+++ b/gcc/testsuite/gcc.target/riscv/pr119971.c
@@ -0,0 +1,24 @@
+/* { dg-do compile { target rv64 } } */
+/* { dg-options "-march=rv64gcb -mabi=lp64" } */
+/* { dg-skip-if "" { *-*-* } { "-O0" "-g" "-Oz" "-Os" } } */
+
+__attribute__ ((noipa)) unsigned
+foo (unsigned b, unsigned e, unsigned i)
+{
+ e >>= b;
+ i >>= e & 31;
+ return i & 1;
+}
+
+int main()
+{
+ if (foo (0x18, 0xfe000000, 0x40000000) != 1)
+ __builtin_abort ();
+ return 0;
+}
+
+/* { dg-final { scan-assembler-times "andi\t" 1 } } */
+/* { dg-final { scan-assembler-times "srlw\t" 2 } } */
+/* { dg-final { scan-assembler-not "bext\t" } } */
+
+
diff --git a/gcc/testsuite/gcc.target/riscv/zbb-rol-ror-03.c
b/gcc/testsuite/gcc.target/riscv/zbb-rol-ror-03.c
index f85c20eb74a..3121cb6bff4 100644
--- a/gcc/testsuite/gcc.target/riscv/zbb-rol-ror-03.c
+++ b/gcc/testsuite/gcc.target/riscv/zbb-rol-ror-03.c
@@ -10,7 +10,7 @@ unsigned int rol(unsigned int rs1, unsigned int rs2)
}
unsigned int ror(unsigned int rs1, unsigned int rs2)
{
- int shamt = rs2 & (64 - 1);
+ int shamt = rs2 & (32 - 1);
return (rs1 >> shamt) | (rs1 << ((32 - shamt) & (32 - 1)));
}
