[SelectionDAG] Use unaligned store/load to move AVX registers onto stack for insertelement
#82130
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-backend-x86 @llvm/pr-subscribers-llvm-selectiondag Author: Manish Kausik H (Nirhar) ChangesPrior to this patch, SelectionDAG generated aligned move onto stacks for AVX registers when the function was marked as a no-realign-stack function. This lead to misalignment between the stack and the instruction generated. This patch fixes the issue. There was a similar issue reported for Full diff: https://github.com/llvm/llvm-project/pull/82130.diff 2 Files Affected:
diff --git a/llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp b/llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
index 892bfbd62f0d02..e58adf867ac790 100644
--- a/llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
@@ -363,6 +363,19 @@ SDValue SelectionDAGLegalize::ExpandConstant(ConstantSDNode *CP) {
return Result;
}
+// Helper function that generates an MMO that considers the alignment of the
+// stack, and the size of the stack object
+static MachineMemOperand *getStackAlignedMMO(SDValue StackPtr,
+ MachineFunction &MF,
+ bool isObjectScalable) {
+ auto &MFI = MF.getFrameInfo();
+ int FI = cast<FrameIndexSDNode>(StackPtr)->getIndex();
+ MachinePointerInfo PtrInfo = MachinePointerInfo::getFixedStack(MF, FI);
+ uint64_t ObjectSize = isObjectScalable ? ~UINT64_C(0) : MFI.getObjectSize(FI);
+ return MF.getMachineMemOperand(PtrInfo, MachineMemOperand::MOStore,
+ ObjectSize, MFI.getObjectAlign(FI));
+}
+
/// Some target cannot handle a variable insertion index for the
/// INSERT_VECTOR_ELT instruction. In this case, it
/// is necessary to spill the vector being inserted into to memory, perform
@@ -384,23 +397,23 @@ SDValue SelectionDAGLegalize::PerformInsertVectorEltInMemory(SDValue Vec,
EVT VT = Tmp1.getValueType();
EVT EltVT = VT.getVectorElementType();
SDValue StackPtr = DAG.CreateStackTemporary(VT);
-
- int SPFI = cast<FrameIndexSDNode>(StackPtr.getNode())->getIndex();
+ MachineMemOperand *AlignedMMO = getStackAlignedMMO(
+ StackPtr, DAG.getMachineFunction(), EltVT.isScalableVector());
// Store the vector.
- SDValue Ch = DAG.getStore(
- DAG.getEntryNode(), dl, Tmp1, StackPtr,
- MachinePointerInfo::getFixedStack(DAG.getMachineFunction(), SPFI));
+ SDValue Ch = DAG.getStore(DAG.getEntryNode(), dl, Vec, StackPtr, AlignedMMO);
SDValue StackPtr2 = TLI.getVectorElementPointer(DAG, StackPtr, VT, Tmp3);
// Store the scalar value.
- Ch = DAG.getTruncStore(
- Ch, dl, Tmp2, StackPtr2,
- MachinePointerInfo::getUnknownStack(DAG.getMachineFunction()), EltVT);
- // Load the updated vector.
- return DAG.getLoad(VT, dl, Ch, StackPtr, MachinePointerInfo::getFixedStack(
- DAG.getMachineFunction(), SPFI));
+ Ch = DAG.getTruncStore(Ch, dl, Tmp2, StackPtr2, EltVT, AlignedMMO);
+
+ Align ElementAlignment = std::min(cast<StoreSDNode>(Ch)->getAlign(),
+ DAG.getDataLayout().getPrefTypeAlign(
+ VT.getTypeForEVT(*DAG.getContext())));
+
+ return DAG.getLoad(VT, dl, Ch, StackPtr, MachinePointerInfo(),
+ ElementAlignment);
}
SDValue SelectionDAGLegalize::ExpandINSERT_VECTOR_ELT(SDValue Vec, SDValue Val,
@@ -1378,19 +1391,6 @@ void SelectionDAGLegalize::LegalizeOp(SDNode *Node) {
}
}
-// Helper function that generates an MMO that considers the alignment of the
-// stack, and the size of the stack object
-static MachineMemOperand *getStackAlignedMMO(SDValue StackPtr,
- MachineFunction &MF,
- bool isObjectScalable) {
- auto &MFI = MF.getFrameInfo();
- int FI = cast<FrameIndexSDNode>(StackPtr)->getIndex();
- MachinePointerInfo PtrInfo = MachinePointerInfo::getFixedStack(MF, FI);
- uint64_t ObjectSize = isObjectScalable ? ~UINT64_C(0) : MFI.getObjectSize(FI);
- return MF.getMachineMemOperand(PtrInfo, MachineMemOperand::MOStore,
- ObjectSize, MFI.getObjectAlign(FI));
-}
-
SDValue SelectionDAGLegalize::ExpandExtractFromVectorThroughStack(SDValue Op) {
SDValue Vec = Op.getOperand(0);
SDValue Idx = Op.getOperand(1);
@@ -1488,24 +1488,27 @@ SDValue SelectionDAGLegalize::ExpandInsertToVectorThroughStack(SDValue Op) {
EVT VecVT = Vec.getValueType();
EVT SubVecVT = Part.getValueType();
SDValue StackPtr = DAG.CreateStackTemporary(VecVT);
- int FI = cast<FrameIndexSDNode>(StackPtr.getNode())->getIndex();
- MachinePointerInfo PtrInfo =
- MachinePointerInfo::getFixedStack(DAG.getMachineFunction(), FI);
+ MachineMemOperand *AlignedMMO = getStackAlignedMMO(
+ StackPtr, DAG.getMachineFunction(), VecVT.isScalableVector());
// First store the whole vector.
- SDValue Ch = DAG.getStore(DAG.getEntryNode(), dl, Vec, StackPtr, PtrInfo);
+ SDValue Ch = DAG.getStore(DAG.getEntryNode(), dl, Vec, StackPtr, AlignedMMO);
// Then store the inserted part.
SDValue SubStackPtr =
TLI.getVectorSubVecPointer(DAG, StackPtr, VecVT, SubVecVT, Idx);
// Store the subvector.
- Ch = DAG.getStore(
- Ch, dl, Part, SubStackPtr,
- MachinePointerInfo::getUnknownStack(DAG.getMachineFunction()));
+ Ch = DAG.getStore(Ch, dl, Part, SubStackPtr, AlignedMMO);
+
+ Align ElementAlignment =
+ std::min(cast<StoreSDNode>(Ch)->getAlign(),
+ DAG.getDataLayout().getPrefTypeAlign(
+ Op.getValueType().getTypeForEVT(*DAG.getContext())));
// Finally, load the updated vector.
- return DAG.getLoad(Op.getValueType(), dl, Ch, StackPtr, PtrInfo);
+ return DAG.getLoad(Op.getValueType(), dl, Ch, StackPtr, MachinePointerInfo(),
+ ElementAlignment);
}
SDValue SelectionDAGLegalize::ExpandVectorBuildThroughStack(SDNode* Node) {
diff --git a/llvm/test/CodeGen/X86/unaligned-insert-into-vector-through-stack.ll b/llvm/test/CodeGen/X86/unaligned-insert-into-vector-through-stack.ll
new file mode 100644
index 00000000000000..01e4d02acda18e
--- /dev/null
+++ b/llvm/test/CodeGen/X86/unaligned-insert-into-vector-through-stack.ll
@@ -0,0 +1,18 @@
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py UTC_ARGS: --version 4
+; RUN: llc < %s -mtriple=x86_64-unknown-linux-gnu | FileCheck %s
+
+define <8 x i32> @foo(<8 x i32> %arg1, i32 %n) #0 {
+; CHECK-LABEL: foo:
+; CHECK: # %bb.0: # %entry
+; CHECK-NEXT: vmovups %ymm0, -{{[0-9]+}}(%rsp)
+; CHECK-NEXT: # kill: def $edi killed $edi def $rdi
+; CHECK-NEXT: andl $7, %edi
+; CHECK-NEXT: movl $42, -40(%rsp,%rdi,4)
+; CHECK-NEXT: vmovups -{{[0-9]+}}(%rsp), %ymm0
+; CHECK-NEXT: retq
+entry:
+ %a = insertelement <8 x i32> %arg1, i32 42, i32 %n
+ ret <8 x i32> %a
+}
+
+attributes #0 = { "no-realign-stack" "target-cpu"="haswell" }
|
|
We seem to have 2 different functions that essentially do the same job of spilling the vector onto the stack for inserting into it, which are: and As a suggestion for a different PR, I think we can get rid of one of them, or call one through the other. |
RKSimon
reviewed
Feb 18, 2024
…ack for `insertelement` Prior to this patch, SelectionDAG generated aligned move onto stacks for AVX registers when the function was marked as a no-realign-stack function. This lead to misalignment between the stack and the instruction generated. This patch fixes the issue. There was a similar issue reported for `extractelement` which was fixed in #a6614ec5b7c1dbfc4b847884c5de780cf75e8e9c
Nirhar
force-pushed
the
alignment-error-2
branch
from
2f0989e
to
92a4d89
Compare
You can test this locally with the following command:git-clang-format --diff d2a26a7bd5fc7cc5752337b7f4f999642feb37dc 92a4d898c7c59e18e8527687eef606622c663c6c -- llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cppView the diff from clang-format here.diff --git a/llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp b/llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
index 177784ab6a..195a74da12 100644
--- a/llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
@@ -404,7 +404,6 @@ SDValue SelectionDAGLegalize::PerformInsertVectorEltInMemory(SDValue Vec,
// Store the vector.
SDValue Ch = DAG.getStore(DAG.getEntryNode(), dl, Vec, StackPtr, AlignedMMO);
-
SDValue StackPtr2 = TLI.getVectorElementPointer(DAG, StackPtr, VT, Tmp3);
// Store the scalar value.
|
|
ExpandInsertToVectorThroughStack is for INSERT_SUBVECTOR not INSERT_VECTOR_ELT, but I agree we can probably share more of the code between them. |
|
We have about 10 Aarch64 Unit tests failing, all of them due to the following assertion failure: Is there a way to print the rules, so that I can investigate what those rules contain? |
This will be unrelated to this patch |
arsenm
reviewed
Feb 26, 2024
Comment on lines
+1509
to
+1510
| DAG.getDataLayout().getPrefTypeAlign( | ||
| Op.getValueType().getTypeForEVT(*DAG.getContext()))); |
There was a problem hiding this comment.
This should use the alignment of the actual frame index, not query the type preferred align
| DAG.getDataLayout().getPrefTypeAlign( | ||
| VT.getTypeForEVT(*DAG.getContext()))); | ||
|
|
||
| return DAG.getLoad(VT, dl, Ch, StackPtr, MachinePointerInfo(), |
There was a problem hiding this comment.
Can't we represent this as FrameIndex base with unknown offset?
There was a problem hiding this comment.
Can you please elaborate on what is it that you want to represent this way? My knowledge on this API is still limited, hence I would not have understood your intent.
There was a problem hiding this comment.
This is a reload of the entire vector, so there isn't any offset to consider. That is, the original MachinePointerInfo was correct. You should be able to use the MachinePointerInfo::getFixedStack as before
arsenm
reviewed
Mar 5, 2024
| DAG.getDataLayout().getPrefTypeAlign( | ||
| VT.getTypeForEVT(*DAG.getContext()))); | ||
|
|
||
| return DAG.getLoad(VT, dl, Ch, StackPtr, MachinePointerInfo(), |
There was a problem hiding this comment.
This is a reload of the entire vector, so there isn't any offset to consider. That is, the original MachinePointerInfo was correct. You should be able to use the MachinePointerInfo::getFixedStack as before
|
|
||
| Align ElementAlignment = std::min(cast<StoreSDNode>(Ch)->getAlign(), | ||
| DAG.getDataLayout().getPrefTypeAlign( | ||
| VT.getTypeForEVT(*DAG.getContext()))); |
There was a problem hiding this comment.
Instead of querying the preferred type alignment, this should use the alignment for the real stack object you have. It just happens that today DAG.CreateStackTemporary uses getPrefTypeAlign, but you should check what the MachineFrameInfo says the alignment is for the frame index
|
@Nirhar reverse-ping |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Prior to this patch, SelectionDAG generated aligned move onto stacks for AVX registers when the function was marked as a no-realign-stack function. This lead to misalignment between the stack and the instruction generated. This patch fixes the issue. There was a similar issue reported for
extractelementwhich was fixed in a6614ec