Hi Jeff,
> On 7/19/2026 6:17 AM, Jin Ma wrote:
> > No new macro-fusion functionality is added. Include both
> > instruction UIDs in dump messages so consecutive matches can be
> > distinguished, and report ZEXTW and ZEXTWS with their actual fusion types.
> > Document existing fusion recognizers consistently to make their RTL
> > relationships and constraints easier to understand.
> >
> > Keep each table entry tied to one fusion operation and use the common
> > enablement helper.
> >
> > gcc/ChangeLog:
> >
> > * config/riscv/riscv-fusion.cc (riscv_fuse_zextw_common): New function.
> > (riscv_fuse_zextw): Likewise.
> > (riscv_fuse_zextws): Use riscv_fuse_zextw_common and document RTL
> > relationships and constraints.
> > (riscv_fuse_zexth): Use riscv_fuse_zextw_common.
> > (riscv_fuse_ldindexed): Document RTL relationships and constraints.
> > (riscv_fuse_expanded_ld): Likewise.
> > (riscv_fuse_ldpreincrement): Likewise.
> > (riscv_fuse_lui_addi): Likewise.
> > (riscv_fuse_auipc_addi): Likewise.
> > (riscv_fuse_lui_ld): Likewise.
> > (riscv_fuse_auipc_ld): Likewise.
> > (riscv_fuse_cache_aligned_std): Likewise.
> > (riscv_fuse_aligned_std): Likewise.
> > (riscv_fuse_bfext): Likewise.
> > (riscv_fuse_b_alui): Likewise.
> > (riscv_fusion_entry::op): Rename from op_flags and use an enum.
> > (riscv_fusion_table): Split ZEXTW and ZEXTWS entries.
> > (riscv_macro_fusion_pair_p): Use riscv_fusion_enabled_p and print
> > instruction UIDs.
> >
> > gcc/testsuite/ChangeLog:
> >
> > * gcc.target/riscv/fusion-zextw-2.c: Expect the ZEXTW dump name.
> > * gcc.target/riscv/fusion-zextw.c: Likewise.
> > ---
> >
> > +/* Check for RISCV_FUSE_ZEXTWS fusion.
> > + prev (slli) == (set (reg:DI rd1)
> > + (ashift:DI (reg:DI rs1) (const_int 32)))
> > + curr (srli) == (set (reg:DI rd2)
> > + (lshiftrt:DI (reg:DI rd1) (const_int imm5)))
> > +
> > + Constraints:
> > + rd1 == rd2. */
> > +
> > +static bool
> > +riscv_fuse_zextws (rtx_insn *prev, rtx_insn *curr)
> > +{
> > + if (riscv_fuse_zextw_common (prev, curr, 32, true))
> > + return true;
> > +
> > + return false;
> Isn't this just return riscv_fuse_zext_common (...), ie, no need for a
> conditional in here.
Agreed. I will return the common predicate directly in the next
revision.
> > +}
> > +
> >
> > +
> > +static bool
> > +riscv_fuse_zexth (rtx_insn *prev, rtx_insn *curr)
> > +{
> > + if (riscv_fuse_zextw_common (prev, curr, 48, false))
> > return true;
> >
> > return false;
> Similarly. And I think zextw_common might be better as zext_common
> since I think that routine is getting used for various forms, not just
> those that are zextw.
Agreed. I will make the same simplification for the other wrappers
and rename the helper to riscv_fuse_zext_common in the next revision.
> > }
> >
> >
> >
> > @@ -700,9 +826,8 @@ typedef bool (*fusion_checker_fn) (rtx_insn *, rtx_insn
> > *);
> >
> > struct riscv_fusion_entry
> > {
> > - /* The fusion operation flag to check enablement. For entries that
> > - check multiple flags (ZEXTW/ZEXTWS), use the bitwise OR. */
> > - unsigned int op_flags;
> > + /* The fusion operation to check enablement. */
> > + enum riscv_fusion_pairs op;
> This seems wrong. We're not dealing with an enumeration here, but a
> bitmask of enabled fusion capabilities Right?
>
riscv_get_fusible_ops () does return a bitmask. However, the op
member of each table entry is intended to identify exactly one fusion
operation, rather than an arbitrary combination of fusion capabilities.
The original design intent was that each checker stored in
fusion_checker_fn checker is controlled by exactly one bit. Splitting
ZEXTW and ZEXTWS and using an enum here follows this principle: each
fusion pair should be explicit, minimal, and non-overlapping.
Different CPUs may have subtle differences in their fusion capabilities.
Keeping each fusion pair independent allows the same pair to be enabled
by multiple CPUs, while allowing each CPU to select any combination of
the pairs it supports. Although preserving this separation is not
always easy, it avoids mixing multiple enablement bits in one table
entry and keeps the bit, checker, and dump name in a one-to-one
relationship.
Would it be reasonable to retain the enum to preserve this
one-bit-per-checker invariant?
> Basically OK. Just those minor issues I see.
Thanks for the review. Any other questions or suggestions?
BR,
Jin
