Segher -- there's a combine question near the end...
On 3/23/25 8:43 PM, Bohan Lei wrote:
The combine pass can generate an index like (and:DI (mult:DI (reg:DI)
(const_int scale)) (const_int mask)) when XTheadMemIdx is available.
LRA may pull it out, and thus a splitter is needed when Zba is not
available.
A similar splitter were introduced when XTheadMemIdx support was added,
but removed in commit 31c3c5d. The new splitter in this new patch is
based on the removed one.
gcc/ChangeLog:
* config/riscv/thead.md (*th_memidx_operand): New splitter.
gcc/testsuite/ChangeLog:
* gcc.target/riscv/xtheadmemidx-bug.c: New test.
---
gcc/config/riscv/thead.md | 21 +++++++++++++++++++
.../gcc.target/riscv/xtheadmemidx-bug.c | 13 ++++++++++++
2 files changed, 34 insertions(+)
create mode 100644 gcc/testsuite/gcc.target/riscv/xtheadmemidx-bug.c
diff --git a/gcc/config/riscv/thead.md b/gcc/config/riscv/thead.md
index d816f3b86dd..20e82e68df2 100644
--- a/gcc/config/riscv/thead.md
+++ b/gcc/config/riscv/thead.md
@@ -458,6 +458,27 @@ (define_insn "*th_mempair_load_zero_extendsidi2"
;; XTheadMemIdx
+;; Help reload to add a displacement for the base register.
+;; In the case `zext(*(uN*))(base+((rN<<1)&0x1fffffffe))` LRA splits
+;; off two new instructions: a) `new_base = base + disp`, and
+;; b) `index = (rN<<1)&0x1fffffffe`. The index calculation has no
+;; corresponding instruction pattern and needs this insn_and_split
+;; to recover.
+
+(define_insn_and_split "*th_memidx_operand"
+ [(set (match_operand:DI 0 "register_operand" "=r")
+ (and:DI (ashift:DI (match_operand:DI 1 "register_operand" "r")
+ (match_operand:QI 2 "imm123_operand" "Ds3"))
+ (match_operand 3 "const_int_operand" "n")))]
+ "TARGET_64BIT && TARGET_XTHEADMEMIDX && (lra_in_progress || reload_completed)
+ && (INTVAL (operands[3]) >> INTVAL (operands[2])) == 0xffffffff"
So this is a nasty little problem and touches on a deeper issue.
The core problem is that combine doesn't know anything about
constraints. It works with predicates and conditions. So combine has
no idea if it's creating an ASM with operands that can't be handled. As
a result combine has to be careful with ASMs.
In can_combine_p we have:
/* Can't merge an ASM_OPERANDS. */
|| GET_CODE (src) == ASM_OPERANDS
Which is part of a large conditional indicating when we can't combine
two insns together. So I would have expected it to reject this insn for
combining:
(insn 12 11 0 2 (set (mem/f:DI (reg/v/f:DI 138 [ e ]) [2 *e_6+0 S8 A64])
(asm_operands:DI ("") ("=A") 0 [
(mem/f:DI (reg/v/f:DI 138 [ e ]) [2 *e_6+0 S8 A64])
]
[
(asm_input:DI ("A") j.c:8)
]
[] j.c:8)) "j.c":8:3 -1
(expr_list:REG_DEAD (reg/v/f:DI 138 [ e ])
(nil)))
So the natural question is why did insn 12 participate in combination at
all:
Trying 11 -> 12:
11: r138:DI=r145:DI+r144:DI
REG_DEAD r145:DI
REG_DEAD r144:DI
12: [r138:DI]=asm_operands
REG_DEAD r138:DI
Failed to match this instruction:
(set (mem/f:DI (plus:DI (reg/f:DI 145 [ b ])
(reg:DI 144 [ _4 ])) [2 *e_6+0 S8 A64])
(asm_operands:DI ("") ("=A") 0 [
(mem/f:DI (plus:DI (reg/f:DI 145 [ b ])
(reg:DI 144 [ _4 ])) [2 *e_6+0 S8 A64])
]
[
(asm_input:DI ("A") j.c:8)
]
[] j.c:8))
allowing combination of insns 11 and 12
original costs 4 + 36 = 40
replacement cost 36
deferring deletion of insn with uid = 11.
modifying insn i3 12: [r145:DI+r144:DI]=asm_operands
REG_DEAD r144:DI
REG_DEAD r145:DI
deferring rescan insn with uid = 12.
So without a deep dive, my question is should this have been rejected
for combination before we even got here? I just don't see how combine
can do anything with asms other than replacing one pseudo with another
since combine doesn't have any notion of constraints.
Segher, comments?
jeff
