Re: [PATCH v5 03/18] RISC-V: Add SUB_SEQZ macro-fusion recognition

Hi Jeff,

> On 8/13/2026 11:56 PM, Jin Ma wrote:
> > Recognize SUB/SUBW followed by SEQZ/SNEZ when the operations have
> > the required producer-consumer and destination relationships.  Leave
> > the fusion disabled by default.
> >
> > gcc/ChangeLog:
> >
> > 	* config/riscv/riscv-fusion.cc (riscv_insn_is_sub_type_p): New
> > 	function.
> > 	(riscv_fuse_sub_seqz): Likewise.
> > 	(riscv_fusion_table): Add RISCV_FUSE_SUB_SEQZ.
> > 	* config/riscv/riscv-protos.h (enum riscv_fusion_pairs): Add
> > 	RISCV_FUSE_SUB_SEQZ.
> >
> > gcc/testsuite/ChangeLog:
> >
> > 	* gcc.target/riscv/fusion-sub-seqz-snez.c: New test.
> >
> > Signed-off-by: Jin Ma <[email protected]>
> > ---
> >   gcc/config/riscv/riscv-fusion.cc              | 60 +++++++++++++++++
> >   gcc/config/riscv/riscv-protos.h               |  1 +
> >   .../gcc.target/riscv/fusion-sub-seqz-snez.c   | 66 +++++++++++++++++++
> >   3 files changed, 127 insertions(+)
> >   create mode 100644 gcc/testsuite/gcc.target/riscv/fusion-sub-seqz-snez.c
> >
> > diff --git a/gcc/config/riscv/riscv-fusion.cc b/gcc/config/riscv/riscv-fusion.cc
> > index 88735b580f1..758e576706f 100644
> > --- a/gcc/config/riscv/riscv-fusion.cc
> > +++ b/gcc/config/riscv/riscv-fusion.cc
> >
> >
> > +
> > +  if (riscv_insn_is_sub_type_p (prev)
> > +      && get_attr_type (curr) == TYPE_SLT
> > +      && (curr_code == EQ || curr_code == NE)
> > +      && riscv_fuse_same_dest_p (prev_set, curr_set, true)
> > +      && XEXP (curr_src, 1) == const0_rtx)
> > +    return true;
> So mostly good.  More of a question than a "please change this".

Thank you for the review.

> How convenient are you finding using the insn types to simplify the 
> fusion implementation?  I can see the appeal in that you don't have to 
> write custom recognition code to match the relevant RTL?  If it's 
> helpful and the existing types are a good match, then we can keep doing 
> it.  We can also crack existing types into more specific subtypes (as 
> long as we go back and add the new insn types to the various pipeline 
> models).  So for example, it looks like you use SLT type as the first 
> filter, but it matches too many things.  Then you further refine the 
> filter by checking the code.  If we end up doing that a lot, breaking 
> down the insn type further may be helpful.  We could also consider 
> adding new insn attributes specific to fusion.   Anyway, mostly thinking 
> out loud about ways we might be able to simplify things.

I have found insn types useful, but I think they are better suited to
coarse instruction classification than to replacing exact RTL matching.

First, the current machine description has insns that can easily be
misclassified from structural RTL alone.  For example, a normal ADDI
can be:

  addi a0,a0,1

  (set (reg:DI 10 a0)
       (plus:DI (reg:DI 10 a0)
                (const_int 1)))

This is recognized by *adddi3 in riscv.md and has TYPE_ARITH.  On the
other hand, consider this bit-manipulation insn:

  binvi a0,a0,63

Its RTL is:

  (set (reg:DI 10 a0)
       (plus:DI (reg:DI 10 a0)
                (const_int -9223372036854775808
                           [0x8000000000000000])))

It is recognized by *xor_for_plus_minint in bitmanip.md and has
TYPE_BITMANIP.  Both insns have the top-level RTL form PLUS with a
CONST_INT.  Checking only the RTL code could therefore mistake the
BINVI for an ADDI.  The TYPE_ARITH check in
riscv_insn_is_addi_type_p prevents that.  Thus, the insn type directly
reflects the recognized machine instruction without requiring the
fusion helper to duplicate all operand constraints from the machine
description.

The issue is not limited to PLUS.  The normal ORI/XORI patterns in
riscv.md use IOR/XOR with a CONST_INT and have TYPE_LOGICAL.  The
*<bit_optab>i<mode> pattern in bitmanip.md uses the same top-level RTL
form to generate BSETI/BINVI, but has TYPE_BITMANIP.  In fact, a BINVI
can come from either that XOR form or the PLUS form above.  Classifying
from RTL codes such as IOR, XOR, or PLUS alone is therefore unreliable,
whereas the insn type directly describes the recognized instruction
class.

Second, the insn type can reject unrelated candidates before detailed
opcode, mode, operand, and register-relationship checks.  Here,
TYPE_SLT is the initial filter, while the EQ/NE and zero-operand checks
still identify SEQZ/SNEZ precisely.

Splitting an existing insn type is technically possible, but it would
require updating every pipeline model that uses that type.  Since this
case needs only a small RTL check, I do not think a new subtype is worth
that cost.

A separate fusion-specific attribute is also possible.  If independent
of the scheduling type, it would not require pipeline model changes and
could provide more precise initial filtering for recurring
single-instruction classes.  However, it still could not describe
dependencies between the pair, register relationships, modes, or
immediate constraints, so it would not replace the RTL checks.  Also,
one instruction can participate in several fusions in different
positions, which is awkward for a single enum attribute.  Every
equivalent MD pattern would also need to maintain the attribute, or
fusions could be silently missed.

I therefore currently lean toward keeping a fusion-specific attribute
as an option rather than introducing one in this series.  For now, using
the existing insn type as an initial filter and shared helpers for exact
RTL and constraint checks seems simpler.  A separate attribute might be
more appropriate if several matchers later need the same finer
classification.

However, this is only my current understanding.  I am not sure whether
this assessment is correct or reasonable, or whether it fits the
preferred long-term GCC design.  Could you provide further guidance on
whether we should split the existing insn types, add a separate fusion
attribute, or continue with the current combination of types and RTL
helpers?  Any other comments would also be very welcome.

> OK once the prerequisites are in (I'll probably look at #3-#18 first, 
> then go back to #2).
>
> jeff

Thanks,
Jin