> I was thinking the whole function would better fit riscv-v.cc rather than
> riscv.cc. Not sure about the new name, I guess you wanted to remove
> "overlap"
> because we also allow "no overlap"? Maybe riscv_widen_constraint or just
> widen_constraint? Mhm, not great either...
Yes, it has overlap and non-overlap, actually it belongs to the concept of
widening. Therefore I leave it with name widen here.
How about riscv_v_widen_constraint_ok here? Which is straight-forward and clear.
> As an additional explanation: Even if we don't have a wide_regno yet, here we
> could also decline operands based on just regno alone. If we decided to
> accept
> only overlapping registers in this constraint, we'd might need to e.g.
> decline
> a regno = v0 right away, even if we don't know wide_regno yet. Otherwise,
> the
> return true here could get us into unsatisfiable situations in LRA later, as
> no wide reg can overlap v0 in the high part.
> When accepting "no overlap" as well, we're good I think.
> But this highlights the difficulty of getting a dynamic filter right.
I will add some thing as comment here in case someone else need to learn in
future.
> Why do we need those here? Can we ever see tuple modes?
Yes, for some cases the tuple modes will hit here.
> Could we turn this into an assert as well?
> Maybe just two lines of asserts then, without an if/else?
Sure thing.
> '{' should go to the next line
Opps.
Pan
-----Original Message-----
From: Robin Dapp <[email protected]>
Sent: Friday, July 3, 2026 5:31 PM
To: Li, Pan2 <[email protected]>; [email protected]
Cc: [email protected]; [email protected]; [email protected];
[email protected]; Chen, Ken <[email protected]>; Liu, Hongtao
<[email protected]>
Subject: Re: [PATCH v1 1/3] RISC-V: Allow RVV register overlap for v[sz]ext.vf2
Hi Pan,
Thanks for taking over here, greatly appreciated!
The patch looks mostly good to me, just a few nits.
> bool
> -riscv_widen_overlap_ok (unsigned int regno, machine_mode mode,
> - unsigned int wide_regno, machine_mode wide_mode)
> +riscv_widen_reg_p (unsigned int regno, machine_mode mode,
> + unsigned int wide_regno, machine_mode wide_mode)
I was thinking the whole function would better fit riscv-v.cc rather than
riscv.cc. Not sure about the new name, I guess you wanted to remove "overlap"
because we also allow "no overlap"? Maybe riscv_widen_constraint or just
widen_constraint? Mhm, not great either...
> {
> /* If the referenced regno is no hard reg, allow everything. */
> if (wide_regno == INVALID_REGNUM)
> return true;
As an additional explanation: Even if we don't have a wide_regno yet, here we
could also decline operands based on just regno alone. If we decided to accept
only overlapping registers in this constraint, we'd might need to e.g. decline
a regno = v0 right away, even if we don't know wide_regno yet. Otherwise, the
return true here could get us into unsatisfiable situations in LRA later, as
no wide reg can overlap v0 in the high part.
When accepting "no overlap" as well, we're good I think.
But this highlights the difficulty of getting a dynamic filter right.
> + if (riscv_tuple_mode_p (mode) || riscv_tuple_mode_p (wide_mode))
> + return false;
Why do we need those here? Can we ever see tuple modes?
> - /* Overlap is only allowed in the highest-numbered part of the wider
> - destination. */
> - if (regno == wide_regno)
> + if (wide_nregs < nregs)
> return false;
Could we turn this into an assert as well?
> + else if (wide_nregs == nregs) { /* Source LMUL < 1. */
'{' should go to the next line
> + gcc_checking_assert (nregs == 1);
Maybe just two lines of asserts then, without an if/else?
--
Regards
Robin
