On Wed, Apr 2, 2025 at 5:35 AM Jeff Law <[email protected]> wrote:
>
>
> Segher -- there's a combine question near the end...
I've created PR119587 to keep track of this issue.
BR
Christoph
>
>
> 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
>
>
