Re: [PATCH v4 2/5] RISC-V: Refine macro-fusion helper predicates

Hi Jeff,

Thank you for the detailed explanation.

> On 7/29/2026 1:17 AM, Jin Ma wrote:
> > Factor common RTL matching out of the existing fusion recognizers and
> > use it to validate register dependencies consistently.  Restrict ADDI
> > matching to immediates encodable by the instruction.
> >
> > gcc/ChangeLog:
> >
> >  * config/riscv/riscv-fusion.cc (riscv_set_extract_word_add_p):
> >  New function.
> >  (riscv_regno): Likewise.
> >  (riscv_fusion_same_dest_p): Handle hard-register SUBREGs and
> >  optionally check the first-source dependency.
> >  (riscv_set_is_addi_p): Check the immediate range.
> >  (riscv_fuse_ldpreincrement): Check the update dependency.
> >  (riscv_fuse_lui_addi): Reuse the ADDI matcher, check the source
> >  dependency, and reject x0 and zero-valued LUI patterns.
> >  (riscv_fuse_auipc_addi): Check the source dependency and reject x0.
> >  (riscv_fuse_auipc_ld): Check the address base dependency.
> >  (riscv_fuse_bfext): Use riscv_regno to validate the source register.
> >
> > gcc/testsuite/ChangeLog:
> >
> >  * gcc.target/riscv/fusion-lui-addi-boundary.c: New test.
> >  * gcc.target/riscv/fusion-lui-addi-dependency-rtl.c: New test.
> > ---
> >   gcc/config/riscv/riscv-fusion.cc              | 154 +++++++++++++++---
> >   .../riscv/fusion-lui-addi-boundary.c          |  61 +++++++
> >   .../riscv/fusion-lui-addi-dependency-rtl.c    | 122 ++++++++++++++
> >   3 files changed, 316 insertions(+), 21 deletions(-)
> >   create mode 100644 gcc/testsuite/gcc.target/riscv/fusion-lui-addi-boundary.c
> >   create mode 100644 gcc/testsuite/gcc.target/riscv/fusion-lui-addi-dependency-rtl.c
> >
> >
> > +static unsigned int
> > +riscv_regno (rtx x)
> > +{
> > +  int regno = true_regnum (x);
> > +  if (regno >= 0)
> > +    return regno;
> > +
> > +  /* An unassigned pseudo has no hard-register mapping yet.  Its lowpart can
> > +     use the pseudo's identity, but other parts cannot.  */
> It's not a big deal IMHO, but you can independently access both SI 
> halves of a DI object.  So it's meaningful to ask for the high part as 
> well.  It'll eventually map down to a hard reg or stack slot of course.  
>   In the before-allocation case these routines are going to over-match, 
> it's kind of inherent in the problem and it's OK to over-match at that 
> phase.
>
> By being aggressive and overmatching prior to reload we've got a much 
> better opportunity to bring fusible instructions together. That 
> over-match will be corrected post-reload if the final register selection 
> isn't suitable for fusion.  This was one of the key things from 
> Artimey's work that made fusion meaningfully more effective which could 
> been seen when we evaluated the fusion counts staticly across spec2017 
> as well as when running on design where we could see the fusion cases 
> reported by the design jump measurably and a small overall gain in 
> performance compared to the baseline before Artimey's changes.
>
> It looks like your patch eliminates that behavior.  Prior to register 
> allocation I would strongly recommend assuming things match.

You are right.  My earlier reasoning was incomplete.  Before register
allocation, the final hard register for a pseudo SUBREG is unknown, so
we should not strictly require the two destinations to map to the same
hard register at that point.  The revised implementation deliberately
relaxes this constraint before allocation and rechecks it afterwards
using the final hard-register assignments.

This issue did not appear to have an observable effect in my previous
testing.  I ran a full SPEC regression after the change and statically
compared the fusion counts in the sched2 dumps; I saw no difference.
Regardless, avoiding this strict constraint before allocation is the
correct choice.

I believe the producer-consumer dependency represented by
`used_in_src_p == true` should still be checked in both scheduling
passes.  This verifies that the previous destination is actually used
as the current instruction's first source; it does not depend on hard
register allocation.  Omitting it in sched1 could recognize unrelated
instructions with no such data dependency as fusion pairs.  I therefore
relax only the same-destination constraint before allocation, while
retaining the real source-dependency check required by `used_in_src_p`.
Is that correct?

> > @@ -93,6 +196,7 @@ riscv_set_is_addi_p (rtx set)
> >     return (GET_CODE (SET_SRC (set)) == PLUS
> >      && REG_P (XEXP (SET_SRC (set), 0))
> >      && CONST_INT_P (XEXP (SET_SRC (set), 1))
> > +   && SMALL_OPERAND (INTVAL (XEXP (SET_SRC (set), 1)))
> >      && REG_P (SET_DEST (set)));
> So I'm curious.  Since we can only see valid insns here, we shouldn't 
> see anything except a SMALL_OPERAND here anyway, right? I'm not 
> objecting to the check, just want to know what caused you to add it.

You are right, and I am sorry.  While checking these matchers against
the ISA descriptions in the hardware specification, I saw the
immediate-range restriction and mechanically added it to the matcher.
I overlooked that only valid instructions reach this point, so the
instruction-generation process already guarantees the range.  No
additional check is needed.

I have removed the `SMALL_OPERAND` check and the boundary test that was
added solely to verify that redundant check.

> My biggest concern here is dropping the assumption that before 
> allocation that the registers will match in the preferred way we want.
>
> Jeff

The revised version preserves aggressive overmatching before register
allocation.  Only real data dependencies are checked in sched1;
constraints that depend on the final hard-register selection are
deferred to sched2.

Thanks,
Jin