Never mind, Jeff. But to be honest it is difficult to tell which part comes from the AI up to a point.
Pan -----Original Message----- From: Jeffrey Law <[email protected]> Sent: Wednesday, April 1, 2026 12:13 AM To: Jeffrey Law <[email protected]>; Li, Pan2 <[email protected]>; [email protected] Cc: [email protected]; [email protected]; [email protected]; Chen, Ken <[email protected]>; Liu, Hongtao <[email protected]> Subject: Re: [PATCH v1 1/3] RISC-V: Allow RVV reg overlap of v[sz]ext.vf2 for m2 Ignore this. I didn't realize I had the AI review stuff in my reply window at the time. On 3/31/2026 10:06 AM, Jeffrey Law wrote: > Main thing that looks wrong: > > - `widening_valid_reg_group_p` is checking the **mode alignment**, not > the actual chosen register number / overlap legality. > - It takes only `operands[0]` and does: > - `riscv_get_v_regno_alignment (GET_MODE (op_0)) == 2` > - But that tells you the natural alignment requirement for the > destination mode, not whether the selected hard reg for operand 0/3 > actually satisfies the intended overlap pattern. > - For `wo21`/`wo22`, legality depends on the concrete register numbers, > e.g. dest even and src odd for the m2/vf2 overlap case. The predicate as > written does not inspect either operand regno. > - So the new `enabled` test appears ineffective / conceptually wrong. > > Related issues: > > - The helper name and comment say “predicate valid overlap or not”, but > it only handles `REG_GROUP_OVERLAP_WO21` by checking destination mode > alignment. That does not match the semantics of the new constraints. > - `wo22` is added but never represented in `reg_group_overlap` attribute > values. The attr only has `"none,wo21"`. That may be intentional if the > attr encodes the pattern class rather than every operand constraint, but > it is confusing and worth clarifying. > - The new alternatives: > - dest: `wo22` > - src: `wo21` > - merge: `vu` / `0` > These already seem to encode the overlap relationship structurally for > the m2 case. If so, the new `enabled` attribute logic may be redundant. > If not, it is insufficient. > - The predicate signature probably needs both dest and src RTXes, or > just the insn pattern operands, and should check actual hard-reg numbers > after allocation/reload context if this is meant to validate overlap. > > Less important, but I’d ask about: > > - The changelog says “Add reg constraint wo21 and wo22” and “Add 2 new > alternative for m2 reg overlap for v[sz]ext.vf2”, but the pattern is > generic. Are these alternatives accidentally offered for modes beyond > LMUL=2? If so, the constraints alone may not restrict to only m2 in the > intended way. > - Wording/docs in `constraints.md` are pretty rough; not a blocker, but > the descriptions are hard to parse. > > So I would not take this as-is. The core validation logic seems > incorrect because it does not test the actual register overlap condition. > > > > On 2/26/2026 12:35 AM, [email protected] wrote: >> From: Pan Li <[email protected]> >> >> According to the RVV 1.0 spec, the widening ops like vzext.vf2 >> could have the source registers overlap on constraint, aka: >> >> ` >> The destination EEW is greater than the source EEW, the source >> EMUL is at least 1, and the overlap is in the highest- >> numbered part of the destination register group. >> ` >> >> This PATCH could like to re-enable it start from vzext.vf2 for >> lmul m2. >> >> gcc/ChangeLog: >> >> * config/riscv/constraints.md (TARGET_VECTOR ? V_REGS : NO_REGS): >> Add reg constraint wo21 and wo22. >> * config/riscv/riscv-protos.h (widening_valid_reg_group_p): Add >> new func decl to predicate valid overlap or not. >> * config/riscv/riscv-v.cc (widening_valid_reg_group_p): Add new >> func impl to predicate valid overlap or not. >> * config/riscv/riscv.md (none,wo21): Add new attr for reg >> overlap insns. >> * config/riscv/vector.md: Add 2 new alternative for m2 reg >> overlap for v[sz]ext.vf2. >> >> Signed-off-by: Pan Li <[email protected]> >> --- >> gcc/config/riscv/constraints.md | 34 +++++++++++++++++++++++++++++++++ >> gcc/config/riscv/riscv-protos.h | 1 + >> gcc/config/riscv/riscv-v.cc | 18 +++++++++++++++++ >> gcc/config/riscv/riscv.md | 3 +++ >> gcc/config/riscv/vector.md | 24 ++++++++++++++--------- >> 5 files changed, 71 insertions(+), 9 deletions(-) >> >> >> >> +(define_register_constraint "wo21" "TARGET_VECTOR ? V_REGS : NO_REGS" >> + "A constraint matches the reg group overlap with highest 1 num in 2 regs." >> + "(regno % 2) == 1") >> +(define_register_constraint "wo22" "TARGET_VECTOR ? V_REGS : NO_REGS" >> + "A constraint matches the reg group overlap in 2 regs." >> + "(regno % 2) == 0") > So this reminds me a bit of how we deal with auto-inc address modes > where we're not allowed to have the same physical register used in the > auto-inc address and as another input. So think a store to memory > where the source register is also used as part of an auto-inc addressing > mode. > > The trick Paul (IIRC) noted is that you can define a set of constraint > pairs say X and Y where X would have just one register and Y would have > all the registers except for one in X. Then you can describe with > constraints the restriction. It's not pretty and not scalable, but it > works for oddballs like pdp11 and h8 where this is a real issue. > > You patch uses the same idea, define paired constraints with disjoint > members. > > But I'm struggling to see how (regno % 2) == [01] is the right test > here. Let's take your table: > >> +;; -----------+------------+------------+----------+ >> +;; | | LMUL = 8 | LMUL = 4 | LMUL = 2 | >> +;; +-----------+------------+------------+----------+ >> +;; | vzext.vf2 | EMUL = 4 | EMUL = 2 | EMUL = 1 | >> +;; +-----------+------------+------------+----------+ >> +;; | | v0-7, v4-7 | v0-3, v2-3 | v0-1, v1 | > If we consider the LMUL=8, EMUL4 case, won't this return true for v0, v2 > when only v4-v7 are OK? Or am I missing something? > > jeff
