Skip to content

[RISCV] Add disjoint or patterns for vwadd[u].vv #136716

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Apr 23, 2025
Merged

[RISCV] Add disjoint or patterns for vwadd[u].vv #136716

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6cd1750
[RISCV] Add disjoint or patterns for vwadd[u].vv
lukel97 Apr 22, 2025
644fe31
Don't propagate disjoint for inreg exts, use or_is_add, match SplatPat
lukel97 Apr 22, 2025
f69c8e6
Update multiclass name to include VX
lukel97 Apr 23, 2025
9599e60
Remove extraneous newline
lukel97 Apr 23, 2025
288fb1f
Fix records
lukel97 Apr 23, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions llvm/include/llvm/Target/TargetSelectionDAG.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1113,6 +1113,10 @@ def not : PatFrag<(ops node:$in), (xor node:$in, -1)>;
def vnot : PatFrag<(ops node:$in), (xor node:$in, immAllOnesV)>;
def ineg : PatFrag<(ops node:$in), (sub 0, node:$in)>;

def or_disjoint : PatFrag<(ops node:$x, node:$y), (or node:$x, node:$y), [{
Copy link
Collaborator

@preames preames Apr 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see the existing or_is_add and add_like in RISCVInstrInfo.td. We can move into generic, but let's do that separately.

return N->getFlags().hasDisjoint();
}]>;

def zanyext : PatFrags<(ops node:$op),
[(zext node:$op),
(anyext node:$op)]>;
Expand Down
4 changes: 3 additions & 1 deletion llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5982,7 +5982,9 @@ SDValue DAGCombiner::hoistLogicOpWithSameOpcodeHands(SDNode *N) {
LegalTypes && !TLI.isTypeDesirableForOp(LogicOpcode, XVT))
return SDValue();
// logic_op (hand_op X), (hand_op Y) --> hand_op (logic_op X, Y)
SDValue Logic = DAG.getNode(LogicOpcode, DL, XVT, X, Y);
SDNodeFlags LogicFlags;
LogicFlags.setDisjoint(N->getFlags().hasDisjoint());
Copy link
Collaborator

@preames preames Apr 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to make sure I understand this right. We've got (or disjoint (ext a), (ext b)) and we're turning that into (ext (or disjoint a, b)). Right?

For zext, this is fine. For sext, this is fine. For the in_reg variants, I am not sure if this is fine or not. What if a and b had the same high bit set and we're doing a sext_in_reg which clears that bit? I think that violates the disjoint and introduces UB which didn't previously exist right?

Copy link
Contributor Author

@lukel97 lukel97 Apr 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agh good catch, alive2 does indeed confirm this: https://alive2.llvm.org/ce/z/W9F5b8. Will fix

SDValue Logic = DAG.getNode(LogicOpcode, DL, XVT, X, Y, LogicFlags);
if (HandOpcode == ISD::SIGN_EXTEND_INREG)
return DAG.getNode(HandOpcode, DL, VT, Logic, N0.getOperand(1));
return DAG.getNode(HandOpcode, DL, VT, Logic);
Expand Down
19 changes: 19 additions & 0 deletions llvm/lib/Target/RISCV/RISCVInstrInfoVSDPatterns.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -912,6 +912,25 @@ defm : VPatWidenBinarySDNode_VV_VX_WV_WX<add, sext_oneuse, "PseudoVWADD">;
defm : VPatWidenBinarySDNode_VV_VX_WV_WX<add, zext_oneuse, "PseudoVWADDU">;
defm : VPatWidenBinarySDNode_VV_VX_WV_WX<add, anyext_oneuse, "PseudoVWADDU">;

// DAGCombiner::hoistLogicOpWithSameOpcodeHands may hoist disjoint ors
Copy link
Collaborator

@preames preames Apr 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have saying this - but remember this only covers scalable types, and that you probably need a follow up patch for the fixed length variant.

// to (ext (or disjoint (a, b)))
multiclass VPatWidenOrDisjoint_VV<SDNode extop, string instruction_name> {
foreach vtiToWti = AllWidenableIntVectors in {
defvar vti = vtiToWti.Vti;
defvar wti = vtiToWti.Wti;
let Predicates = !listconcat(GetVTypePredicates<vti>.Predicates,
GetVTypePredicates<wti>.Predicates) in {
def : Pat<(wti.Vector (extop (vti.Vector (or_disjoint vti.RegClass:$rs2, vti.RegClass:$rs1)))),
Copy link
Collaborator

@preames preames Apr 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not include the splatpat one?

I think you can remove your custom multiclass, and do:

defm : VPatWidenBinarySDNode_VV_VX_WV_WX<add_like, sext_oneuse, "PseudoVWADD">;
defm : VPatWidenBinarySDNode_VV_VX_WV_WX<add_like, zext_oneuse, "PseudoVWADDU">;
defm : VPatWidenBinarySDNode_VV_VX_WV_WX<add_like, anyext_oneuse, "PseudoVWADDU">;

Copy link
Contributor Author

@lukel97 lukel97 Apr 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, I've added a .vx pattern too now.

Unless I'm missing something I don't think I can reuse the VPatWidenBinarySDNode_VV_VX_WV_WX multiclass because it's a different pattern. VPatWidenOrDisjoint_VV matches ext (or a b),VPatWidenBinarySDNode_VV_VX_WV_WX matches or (ext a), (ext b)

(!cast<Instruction>(instruction_name#"_VV_"#vti.LMul.MX)
(wti.Vector (IMPLICIT_DEF)), vti.RegClass:$rs2,
vti.RegClass:$rs1, vti.AVL, vti.Log2SEW, TA_MA)>;
}
}
}
defm : VPatWidenOrDisjoint_VV<sext, "PseudoVWADD">;
defm : VPatWidenOrDisjoint_VV<zext, "PseudoVWADDU">;
defm : VPatWidenOrDisjoint_VV<anyext, "PseudoVWADDU">;

defm : VPatWidenBinarySDNode_VV_VX_WV_WX<sub, sext_oneuse, "PseudoVWSUB">;
defm : VPatWidenBinarySDNode_VV_VX_WV_WX<sub, zext_oneuse, "PseudoVWSUBU">;
defm : VPatWidenBinarySDNode_VV_VX_WV_WX<sub, anyext_oneuse, "PseudoVWSUBU">;
Expand Down
14 changes: 4 additions & 10 deletions llvm/test/CodeGen/RISCV/rvv/vwadd-sdnode.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1417,31 +1417,25 @@ define <vscale x 2 x i32> @vwaddu_vv_disjoint_or_add(<vscale x 2 x i8> %x.i8, <v
ret <vscale x 2 x i32> %add
}

; TODO: We could select vwaddu.vv, but when both arms of the or are the same
; DAGCombiner::hoistLogicOpWithSameOpcodeHands moves the zext above the or.
define <vscale x 2 x i32> @vwaddu_vv_disjoint_or(<vscale x 2 x i16> %x.i16, <vscale x 2 x i16> %y.i16) {
; CHECK-LABEL: vwaddu_vv_disjoint_or:
; CHECK: # %bb.0:
; CHECK-NEXT: vsetvli a0, zero, e16, mf2, ta, ma
; CHECK-NEXT: vor.vv v9, v8, v9
; CHECK-NEXT: vsetvli zero, zero, e32, m1, ta, ma
; CHECK-NEXT: vzext.vf2 v8, v9
; CHECK-NEXT: vwaddu.vv v10, v8, v9
; CHECK-NEXT: vmv1r.v v8, v10
; CHECK-NEXT: ret
%x.i32 = zext <vscale x 2 x i16> %x.i16 to <vscale x 2 x i32>
%y.i32 = zext <vscale x 2 x i16> %y.i16 to <vscale x 2 x i32>
%or = or disjoint <vscale x 2 x i32> %x.i32, %y.i32
ret <vscale x 2 x i32> %or
}

; TODO: We could select vwadd.vv, but when both arms of the or are the same
; DAGCombiner::hoistLogicOpWithSameOpcodeHands moves the zext above the or.
define <vscale x 2 x i32> @vwadd_vv_disjoint_or(<vscale x 2 x i16> %x.i16, <vscale x 2 x i16> %y.i16) {
; CHECK-LABEL: vwadd_vv_disjoint_or:
; CHECK: # %bb.0:
; CHECK-NEXT: vsetvli a0, zero, e16, mf2, ta, ma
; CHECK-NEXT: vor.vv v9, v8, v9
; CHECK-NEXT: vsetvli zero, zero, e32, m1, ta, ma
; CHECK-NEXT: vsext.vf2 v8, v9
; CHECK-NEXT: vwadd.vv v10, v8, v9
; CHECK-NEXT: vmv1r.v v8, v10
; CHECK-NEXT: ret
%x.i32 = sext <vscale x 2 x i16> %x.i16 to <vscale x 2 x i32>
%y.i32 = sext <vscale x 2 x i16> %y.i16 to <vscale x 2 x i32>
Expand Down
Loading