On 9/2/24 7:52 AM, Jovan Vukic wrote:
The patch adds a new instruction pattern to handle conditional branches
with equality checks between shifted arithmetic operands. This pattern
optimizes the use of shifted constants (with trailing zeros), making it
more efficient.
For the C code:
void f5(long long a) {
if ((a & 0x2120000) == 0x2000000)
g();
}
before the patch, the assembly code was:
f5:
li a5,34734080
and a0,a0,a5
li a5,33554432
beq a0,a5,.L21
ret
and after the patch the assembly is:
f5:
srli a5,a0,17
andi a5,a5,265
li a4,256
beq a5,a4,.L21
ret
Tested on both RV32 and RV64 with no regressions.
2024-09-02 Jovan Vukic <jovan.vu...@rt-rk.com>
gcc/ChangeLog:
PR target/113248
* config/riscv/riscv.md
(*branch<ANYI:mode>_shiftedarith_equals_shifted): New pattern.
gcc/testsuite/ChangeLog:
PR target/113248
* gcc.target/riscv/branch-1.c: Additional tests.
---
gcc/config/riscv/riscv.md | 32 +++++++++++++++++++++++
gcc/testsuite/gcc.target/riscv/branch-1.c | 16 +++++++++---
2 files changed, 45 insertions(+), 3 deletions(-)
diff --git a/gcc/config/riscv/riscv.md b/gcc/config/riscv/riscv.md
index 3289ed2155a..c98a66dbc7c 100644
--- a/gcc/config/riscv/riscv.md
+++ b/gcc/config/riscv/riscv.md
@@ -3126,6 +3126,38 @@
}
[(set_attr "type" "branch")])
+(define_insn_and_split "*branch<ANYI:mode>_shiftedarith_equals_shifted"
+ [(set (pc)
+ (if_then_else (match_operator 1 "equality_operator"
+ [(and:ANYI (match_operand:ANYI 2 "register_operand" "r")
+ (match_operand 3 "shifted_const_arith_operand"
"i"))
+ (match_operand 4 "shifted_const_arith_operand" "i")])
+ (label_ref (match_operand 0 "" ""))
+ (pc)))
+ (clobber (match_scratch:X 5 "=&r"))
+ (clobber (match_scratch:X 6 "=&r"))]
So match_operator works and I'm guessing you used it due to the its use
in the existing *branch<ANYI:mode>_shiftedarith_equals_zero pattern.
It's worth noting there is a newer way which is usually slightly simpler
than a match_operator. Specifically code iterators. After defining the
iterator, you can use it in a pattern just like a simple RTL code. So
as an example:
(define_insn "*<optab><mode>3"
[(set (match_operand:X 0 "register_operand" "=r,r")
(any_or:X (match_operand:X 1 "register_operand" "%r,r")
(match_operand:X 2 "arith_operand" " r,I")))]
""
"<insn>%i2\t%0,%1,%2"
[(set_attr "type" "logical")
(set_attr "mode" "<MODE>")])
Note the "any_or" reference. That's a code iterator that expands to ior
and xor, trivially allowing the pattern to match both cases. The <insn>
and <optab> will map the xor/ior to the right assembly mnemonic and the
optab name. The definition of any_or, as well as the mapping iterators
are all kept in iterators.md.
I don't think you necessary need to change your patch, I'm just pointing
out there's a newer way to do this rather than use a match_operator.
--
So from a correctness standpoint, after further review, I'm not as
concerned about the subreg in the output template. I'm a little
concerned that this pattern will generate unrecognized insns.
The pattern uses shifted_const_arith_operand, which is good as it
validates that the constant, if normalized by shifting away its trailing
zeros fits in a simm12.
But the normalization you're doing on the two constants is limited by
the smaller of trailing zero counts. So operands2 might be 0x8100 which
requires an 8 bit shift for normalization. operands3 might be 0x81000
which requires a 12 bit shift for normalization. In that case we'll use
8 as our shift count for normalization, resulting in:
0x8100 >> 8 = 0x81, a valid small operand
0x81000 >> 8 = 0x810, not a valid small operand.
I think that'll generate invalid RTL at split time.
What I think you need to do is in the main predicate (the same place
you're currently !SMALL_OPERAND (INTVAL (operands[3]))), you'll need to
check that both operands are SMALL_OPERAND after normalization.
I'd suggest putting that check into a little function rather than trying
to do it all inline. I wouldn't be surprised if you could have that
little function also be used in the C fragment which sets up operands8..10.
But I think you're on a good path.
Jeff
ps. Assuming I'm right, it would seem like a negative test with 0x8100
and 0x81000 as the constants would be useful.
