-
Notifications
You must be signed in to change notification settings - Fork 14.5k
[RISCV] Handle scalable ops with < EEW / 2 narrow types in combineBinOp_VLToVWBinOp_VL #84158
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.
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13652,16 +13652,6 @@ struct NodeExtensionHelper { | |
if (!VT.isVector()) | ||
break; | ||
|
||
SDValue NarrowElt = OrigOperand.getOperand(0); | ||
MVT NarrowVT = NarrowElt.getSimpleValueType(); | ||
|
||
unsigned ScalarBits = VT.getScalarSizeInBits(); | ||
unsigned NarrowScalarBits = NarrowVT.getScalarSizeInBits(); | ||
|
||
// Ensure the extension's semantic is equivalent to rvv vzext or vsext. | ||
if (ScalarBits != NarrowScalarBits * 2) | ||
break; | ||
|
||
SupportsZExt = Opc == ISD::ZERO_EXTEND; | ||
SupportsSExt = Opc == ISD::SIGN_EXTEND; | ||
|
||
|
@@ -14112,7 +14102,9 @@ static SDValue combineBinOp_VLToVWBinOp_VL(SDNode *N, | |
TargetLowering::DAGCombinerInfo &DCI, | ||
const RISCVSubtarget &Subtarget) { | ||
SelectionDAG &DAG = DCI.DAG; | ||
if (DCI.isBeforeLegalize()) | ||
// Don't perform this until types are legalized and any legal i1 types are | ||
// custom lowered to avoid introducing unselectable V{S,Z}EXT_VLs. | ||
if (DCI.isBeforeLegalizeOps()) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure this is 100% reliable. Its theoretically possible for an i1 vector to be created by the DAG combiner after legalize ops. The last DAG combine stage also runs the legalizer on every node as part of its worklist. So its not illegal for an i1 zext to created as it would get legalized before isel. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe it is better for us to still check whether the op is legal. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've updated the PR to instead check that the narrow element type isn't i1 across the different possible extend ops |
||
return SDValue(); | ||
|
||
if (!NodeExtensionHelper::isSupportedRoot(N)) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If after the prior change which moves this transform after legalize types, the only case which needs this restriction to keep the transform between legalize types and legalize ops is the i1 vector case, why not simply check if the narrow vt is a i1 vector here? Wouldn't that be less disruptive than moving the combine after legalize ops?
Note that you should also be asserting that both narrow and wide are legal types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved it to after the legalize vector ops phase since we weren't checking for i1 vectors in any of the other _VL nodes. So I think there was already an implicit invariant here that the combine would only run after legalize ops, and it seemed safer to just be explicit about it.
Since the combine was already happening after legalize ops for the _VL nodes, this should only affect the ISD::ADD/SUB/MUL nodes that were added in #76785
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've moved the narrow type assert in 0ef61ed so that we now check the narrow type for all extend node types, and we have an assert that the wide type is legal here:
llvm-project/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
Line 13755 in 74ea8fe