-
Notifications
You must be signed in to change notification settings - Fork 13.6k
[RISCV] Prefer whole register loads and stores when VL=VLMAX #75531
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 all commits
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 |
---|---|---|
|
@@ -9838,6 +9838,19 @@ RISCVTargetLowering::lowerFixedLengthVectorLoadToRVV(SDValue Op, | |
MVT XLenVT = Subtarget.getXLenVT(); | ||
MVT ContainerVT = getContainerForFixedLengthVector(VT); | ||
|
||
// If we know the exact VLEN and our fixed length vector completely fills | ||
// the container, use a whole register load instead. | ||
const auto [MinVLMAX, MaxVLMAX] = | ||
RISCVTargetLowering::computeVLMAXBounds(ContainerVT, Subtarget); | ||
if (MinVLMAX == MaxVLMAX && MinVLMAX == VT.getVectorNumElements() && | ||
getLMUL1VT(ContainerVT).bitsLE(ContainerVT)) { | ||
SDValue NewLoad = | ||
DAG.getLoad(ContainerVT, DL, Load->getChain(), Load->getBasePtr(), | ||
Load->getMemOperand()); | ||
SDValue Result = convertFromScalableVector(VT, NewLoad, DAG, Subtarget); | ||
return DAG.getMergeValues({Result, NewLoad.getValue(1)}, DL); | ||
} | ||
|
||
SDValue VL = getVLOp(VT.getVectorNumElements(), ContainerVT, DL, DAG, Subtarget); | ||
|
||
bool IsMaskOp = VT.getVectorElementType() == MVT::i1; | ||
|
@@ -9882,12 +9895,22 @@ RISCVTargetLowering::lowerFixedLengthVectorStoreToRVV(SDValue Op, | |
|
||
MVT ContainerVT = getContainerForFixedLengthVector(VT); | ||
|
||
SDValue VL = getVLOp(VT.getVectorNumElements(), ContainerVT, DL, DAG, | ||
Subtarget); | ||
|
||
SDValue NewValue = | ||
convertToScalableVector(ContainerVT, StoreVal, DAG, Subtarget); | ||
|
||
|
||
// If we know the exact VLEN and our fixed length vector completely fills | ||
// the container, use a whole register store instead. | ||
const auto [MinVLMAX, MaxVLMAX] = | ||
RISCVTargetLowering::computeVLMAXBounds(ContainerVT, Subtarget); | ||
if (MinVLMAX == MaxVLMAX && MinVLMAX == VT.getVectorNumElements() && | ||
getLMUL1VT(ContainerVT).bitsLE(ContainerVT)) | ||
return DAG.getStore(Store->getChain(), DL, NewValue, Store->getBasePtr(), | ||
Store->getMemOperand()); | ||
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 seeing a miscompile on SPEC CPU 2017 502.gcc_r when compiled with I don't have any reduced test case to share at the moment, and looking at the code and the test diffs nothing seems to stick out to me. Will keep investigating 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. Looks like it isn't affected by the actual use of vs1r.v itself, since replacing the patterns to emit vse64.vs still miscompiles. The resulting diff shows that some of these stores are reordered, and the memory VT of the node also changes from 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 wondering if the problem is the memory type in the memory operand. We leave a fixed size memory operand in place for a whole register (i.e. scalable?) store. Maybe this confuses something in AA? I don't think this is wrong per se, but it might be surprising. 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. The ultimate cause of the miscompile was due to a dodgy combine on ISD::STORE which incorrectly removed a scalar load/store pair, assuming they didn't alias with a fixed length vector ISD::STORE. There was nothing wrong in this PR, it just happened to trigger it by lowering to an ISD::STORE node instead of a llvm.riscv.vse intrinsic. Coincidentally on the same day I began investigating this, #83017 was landed which fixes it. I've added a test case here which illustrates it in more detail: 63725ab |
||
|
||
SDValue VL = getVLOp(VT.getVectorNumElements(), ContainerVT, DL, DAG, | ||
Subtarget); | ||
|
||
bool IsMaskOp = VT.getVectorElementType() == MVT::i1; | ||
SDValue IntID = DAG.getTargetConstant( | ||
IsMaskOp ? Intrinsic::riscv_vsm : Intrinsic::riscv_vse, DL, XLenVT); | ||
|
Uh oh!
There was an error while loading. Please reload this page.