[SelectionDAG] Use unaligned store/load to move AVX registers onto stack for `insertelement` #82130

Nirhar · 2024-02-17T20:55:58Z

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 a6614ec

llvmbot · 2024-02-17T20:56:27Z

@llvm/pr-subscribers-backend-x86

@llvm/pr-subscribers-llvm-selectiondag

Author: Manish Kausik H (Nirhar)

Changes

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

Full diff: https://github.com/llvm/llvm-project/pull/82130.diff

2 Files Affected:

(modified) llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp (+35-32)
(added) llvm/test/CodeGen/X86/unaligned-insert-into-vector-through-stack.ll (+18)

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" }

Nirhar · 2024-02-17T20:56:55Z

@RKSimon @danilaml @arsenm for your reviews

Nirhar · 2024-02-17T21:02:52Z

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:

llvm-project/llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp

Line 125 in f81f7a5

SDValue PerformInsertVectorEltInMemory(SDValue Vec, SDValue Val, SDValue Idx,

and

llvm-project/llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp

Line 193 in f81f7a5

SDValue ExpandInsertToVectorThroughStack(SDValue Op);

As a suggestion for a different PR, I think we can get rid of one of them, or call one through the other.

llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp

github-actions · 2024-02-20T15:23:30Z

✅ With the latest revision this PR passed the C/C++ code formatter.

RKSimon · 2024-02-20T15:30:04Z

ExpandInsertToVectorThroughStack is for INSERT_SUBVECTOR not INSERT_VECTOR_ELT, but I agree we can probably share more of the code between them.

Nirhar · 2024-02-20T15:58:21Z

We have about 10 Aarch64 Unit tests failing, all of them due to the following assertion failure:

AArch64Tests: /home/nirhar/Documents/OpenSource/llvm/llvm-project/llvm/include/llvm/CodeGen/GlobalISel/LegalizerInfo.h:576: void llvm::LegalizeRuleSet::aliasTo(unsigned int): Assertion `Rules.empty() && "Aliasing will discard rules"' failed.
 #0 0x00007feec86b1188 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) /home/nirhar/Documents/OpenSource/llvm/llvm-project/llvm/lib/Support/Unix/Signals.inc:723:22
 #1 0x00007feec86b15a9 PrintStackTraceSignalHandler(void*) /home/nirhar/Documents/OpenSource/llvm/llvm-project/llvm/lib/Support/Unix/Signals.inc:798:1
 #2 0x00007feec86aeb49 llvm::sys::RunSignalHandlers() /home/nirhar/Documents/OpenSource/llvm/llvm-project/llvm/lib/Support/Signals.cpp:105:20
 #3 0x00007feec86b0a20 SignalHandler(int) /home/nirhar/Documents/OpenSource/llvm/llvm-project/llvm/lib/Support/Unix/Signals.inc:413:1
 #4 0x00007feec7c42520 (/lib/x86_64-linux-gnu/libc.so.6+0x42520)
 #5 0x00007feec7c969fc pthread_kill (/lib/x86_64-linux-gnu/libc.so.6+0x969fc)
 #6 0x00007feec7c42476 gsignal (/lib/x86_64-linux-gnu/libc.so.6+0x42476)
 #7 0x00007feec7c287f3 abort (/lib/x86_64-linux-gnu/libc.so.6+0x287f3)
 #8 0x00007feec7c2871b (/lib/x86_64-linux-gnu/libc.so.6+0x2871b)
 #9 0x00007feec7c39e96 (/lib/x86_64-linux-gnu/libc.so.6+0x39e96)
#10 0x00007feecb9d2d42 llvm::LegalizeRuleSet::aliasTo(unsigned int) /home/nirhar/Documents/OpenSource/llvm/llvm-project/llvm/include/llvm/CodeGen/GlobalISel/LegalizerInfo.h:577:13
#11 0x00007feecb9d16c1 llvm::LegalizerInfo::aliasActionDefinitions(unsigned int, unsigned int) /home/nirhar/Documents/OpenSource/llvm/llvm-project/llvm/lib/CodeGen/GlobalISel/LegalizerInfo.cpp:319:1
#12 0x00007feecb9d15c4 llvm::LegalizerInfo::getActionDefinitionsBuilder(std::initializer_list<unsigned int>) /home/nirhar/Documents/OpenSource/llvm/llvm-project/llvm/lib/CodeGen/GlobalISel/LegalizerInfo.cpp:305:3
#13 0x00007feecc0d6816 llvm::LegalizeRuleSet::legalFor(std::initializer_list<llvm::LLT>) /home/nirhar/Documents/OpenSource/llvm/llvm-project/llvm/include/llvm/CodeGen/GlobalISel/LegalizerInfo.h:600:21
#14 0x00007feecc0d6816 llvm::AArch64LegalizerInfo::AArch64LegalizerInfo(llvm::AArch64Subtarget const&) /home/nirhar/Documents/OpenSource/llvm/llvm-project/llvm/lib/Target/AArch64/GISel/AArch64LegalizerInfo.cpp:126:0
#15 0x00007feecc2b68d6 std::__uniq_ptr_impl<llvm::LegalizerInfo, std::default_delete<llvm::LegalizerInfo>>::reset(llvm::LegalizerInfo*) /usr/include/c++/11/bits/unique_ptr.h:179:16
#16 0x00007feecc2b68d6 std::unique_ptr<llvm::LegalizerInfo, std::default_delete<llvm::LegalizerInfo>>::reset(llvm::LegalizerInfo*) /usr/include/c++/11/bits/unique_ptr.h:456:12
#17 0x00007feecc2b68d6 llvm::AArch64Subtarget::AArch64Subtarget(llvm::Triple const&, llvm::StringRef, llvm::StringRef, llvm::StringRef, llvm::TargetMachine const&, bool, unsigned int, unsigned int, bool, bool, bool) /home/nirhar/Documents/OpenSource/llvm/llvm-project/llvm/lib/Target/AArch64/AArch64Subtarget.cpp:333:0
#18 0x000055f3624367b3 std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>::_M_data() const /usr/include/c++/11/bits/basic_string.h:195:28
#19 0x000055f3624367b3 std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>::_M_is_local() const /usr/include/c++/11/bits/basic_string.h:230:23
#20 0x000055f3624367b3 std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>::_M_dispose() /usr/include/c++/11/bits/basic_string.h:239:18
#21 0x000055f3624367b3 std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>::~basic_string() /usr/include/c++/11/bits/basic_string.h:672:19
#22 0x000055f3624367b3 (anonymous namespace)::createInstrInfo(llvm::TargetMachine*) /usr/include/c++/11/bits/basic_string.h:671:7
#23 0x000055f362436edf function<InstSizes_STATEPOINT_Test::TestBody()::<lambda(llvm::AArch64InstrInfo&, llvm::MachineFunction&)> > /usr/include/c++/11/bits/std_function.h:437:19
#24 0x000055f362436edf InstSizes_STATEPOINT_Test::TestBody() /home/nirhar/Documents/OpenSource/llvm/llvm-project/llvm/unittests/Target/AArch64/InstSizes.cpp:155:0
#25 0x00007feecbeb09ca testing::Test::Run() /home/nirhar/Documents/OpenSource/llvm/llvm-project/third-party/unittest/googletest/src/gtest.cc:2687:50
#26 0x00007feecbeb09ca testing::Test::Run() /home/nirhar/Documents/OpenSource/llvm/llvm-project/third-party/unittest/googletest/src/gtest.cc:2677:6
#27 0x00007feecbeb0bdd testing::TestInfo::Run() /home/nirhar/Documents/OpenSource/llvm/llvm-project/third-party/unittest/googletest/src/gtest.cc:2836:14
#28 0x00007feecbeb7172 testing::TestSuite::Run() (.part.0) /home/nirhar/Documents/OpenSource/llvm/llvm-project/third-party/unittest/googletest/src/gtest.cc:3017:35
#29 0x00007feecbeb796f testing::internal::UnitTestImpl::RunAllTests() /home/nirhar/Documents/OpenSource/llvm/llvm-project/third-party/unittest/googletest/src/gtest.cc:5921:41
#30 0x00007feecbea9fe1 std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>::_M_data() const /usr/include/c++/11/bits/basic_string.h:195:28
#31 0x00007feecbea9fe1 std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>::c_str() const /usr/include/c++/11/bits/basic_string.h:2321:23
#32 0x00007feecbea9fe1 testing::internal::ScopedPrematureExitFile::~ScopedPrematureExitFile() /home/nirhar/Documents/OpenSource/llvm/llvm-project/third-party/unittest/googletest/src/gtest.cc:5055:26
#33 0x00007feecbea9fe1 testing::UnitTest::Run() /home/nirhar/Documents/OpenSource/llvm/llvm-project/third-party/unittest/googletest/src/gtest.cc:5489:1
#34 0x00007feecc4c51ca main /home/nirhar/Documents/OpenSource/llvm/llvm-project/third-party/unittest/UnitTestMain/TestMain.cpp:56:1
#35 0x00007feec7c29d90 (/lib/x86_64-linux-gnu/libc.so.6+0x29d90)
#36 0x00007feec7c29e40 __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x29e40)
#37 0x000055f36242d7d5 _start (/home/nirhar/Documents/OpenSource/llvm/llvm-project/build/unittests/Target/AArch64/./AArch64Tests+0xa7d5)

Is there a way to print the rules, so that I can investigate what those rules contain?

arsenm · 2024-02-26T15:06:40Z

We have about 10 Aarch64 Unit tests failing, all of them due to the following assertion failure:

AArch64Tests: /home/nirhar/Documents/OpenSource/llvm/llvm-project/llvm/include/llvm/CodeGen/GlobalISel/LegalizerInfo.h:576: void llvm::LegalizeRuleSet::aliasTo(unsigned int): Assertion `Rules.empty() && "Aliasing will discard rules"' failed.

This will be unrelated to this patch

arsenm · 2024-02-26T15:07:58Z

llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp

+               DAG.getDataLayout().getPrefTypeAlign(
+                   Op.getValueType().getTypeForEVT(*DAG.getContext())));


This should use the alignment of the actual frame index, not query the type preferred align

Have modified it! Have a look

arsenm · 2024-02-26T15:08:33Z

llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp

+                                    DAG.getDataLayout().getPrefTypeAlign(
+                                        VT.getTypeForEVT(*DAG.getContext())));
+
+  return DAG.getLoad(VT, dl, Ch, StackPtr, MachinePointerInfo(),


Can't we represent this as FrameIndex base with unknown offset?

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.

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

Okay, have updated it, please have a look!

arsenm · 2024-03-05T10:23:11Z

llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp

+                                    DAG.getDataLayout().getPrefTypeAlign(
+                                        VT.getTypeForEVT(*DAG.getContext())));
+
+  return DAG.getLoad(VT, dl, Ch, StackPtr, MachinePointerInfo(),


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 · 2024-03-05T10:25:41Z

llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp

+
+  Align ElementAlignment = std::min(cast<StoreSDNode>(Ch)->getAlign(),
+                                    DAG.getDataLayout().getPrefTypeAlign(
+                                        VT.getTypeForEVT(*DAG.getContext())));


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

Have a look if my modifications are correct

RKSimon · 2024-04-02T20:15:44Z

@Nirhar reverse-ping

llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp

Nirhar · 2024-07-22T16:40:53Z

ping @RKSimon @arsenm

arsenm

LGTM but It took me a minute to realize what this was doing. The title is misleading. This is not unconditionally using unaligned store, it is avoiding introducing stack realignments

arsenm · 2024-07-22T16:50:30Z

llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp

-  MachinePointerInfo PtrInfo =
-      MachinePointerInfo::getFixedStack(DAG.getMachineFunction(), FI);
+  MachineMemOperand *AlignedMMO = getStackAlignedMMO(
+      StackPtr, DAG.getMachineFunction(), VecVT.isScalableVector());


Do you need to do something to force the alignment down of the underlying stack object?

I had assumed that it was getFixedStack that did the magic of clamping down instruction alignment, but looks like I was wrong. The clamping is done in CreateStackTemporary (in the call to MFI::CreateStackObject) here:

llvm-project/llvm/lib/CodeGen/MachineFrameInfo.cpp

Line 56 in 862d837

Alignment = clampStackAlignment(!StackRealignable, Alignment, StackAlignment);

I think I need to move the call to CreateStackTemporary inside getStackAlignedMMO.

Please ignore the previous comment.

The current version of code generates a MachinePointerInfo and then uses the MachinePointerInfo version of the DAG.getStore() API call, which actually ignores StackAlignment completely. This version of the API call only considers the Alignment of the Object, and it looks like it was meant to only consider that.

When we take the route through getStackAlignedMMO, we transfer the alignment information from MachineFrameInfo to the MachineMemOperand it returns. Now we can use the DAG.getStore MMO API version to maintain the Alignment information.

@arsenm Hope that answers your question

This is a confusing mess of API. I think using an explicit alignment with any getStore would be easier to understand

@arsenm I've modified the patch to only use explicit alignment along with getStore. I'll folllow this patch up with another patch that removes the getStackAlignedMMO function.

llvm/test/CodeGen/X86/unaligned-insert-into-vector-through-stack.ll

Nirhar · 2024-07-26T14:46:39Z

LGTM but It took me a minute to realize what this was doing. The title is misleading. This is not unconditionally using unaligned store, it is avoiding introducing stack realignments

@arsenm Yeah, I agree, I'll modify it. Just to be on the same note, the intention of this patch is to use unaligned move instructions for AVX registers when the function has the no-stack-realign attribute.

arsenm · 2024-07-29T15:58:51Z

llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp

-    Ch = DAG.getStore(
-        Ch, dl, Part, SubStackPtr,
-        MachinePointerInfo::getUnknownStack(DAG.getMachineFunction()));
+    Ch = DAG.getStore(Ch, dl, Part, SubStackPtr, AlignedMMO);


This needs to apply the offset to the MMO?

There is no need for it in the current version of the patch

arsenm · 2024-07-29T15:59:02Z

llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp

-        Ch, dl, Part, SubStackPtr,
-        MachinePointerInfo::getUnknownStack(DAG.getMachineFunction()),
-        VecVT.getVectorElementType());
+    Ch = DAG.getTruncStore(Ch, dl, Part, SubStackPtr,


arsenm · 2024-07-31T20:42:17Z

llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp

@@ -1474,7 +1474,10 @@ SDValue SelectionDAGLegalize::ExpandInsertToVectorThroughStack(SDValue Op) {
      MachinePointerInfo::getFixedStack(DAG.getMachineFunction(), FI);

  // First store the whole vector.
-  SDValue Ch = DAG.getStore(DAG.getEntryNode(), dl, Vec, StackPtr, PtrInfo);
+  Align ElementAlignment =


This is not the element alignment, this is the alignment of the base vector

arsenm · 2024-07-31T20:45:52Z

llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp

@@ -1496,11 +1500,12 @@ SDValue SelectionDAGLegalize::ExpandInsertToVectorThroughStack(SDValue Op) {
    Ch = DAG.getTruncStore(
        Ch, dl, Part, SubStackPtr,
        MachinePointerInfo::getUnknownStack(DAG.getMachineFunction()),
-        VecVT.getVectorElementType());
+        VecVT.getVectorElementType(), ElementAlignment);


I'm still confused about what is going on with the alignment. Can you assert the resulting StoreSDNode's getAlign is the element alignment (the actual element alignment, not the misnamed ElementAlignment here)?

I have added the assert. I hope I am calculating the element alignment correctly(I have called it as PartAlignment, to accomodate when the inserted part is a vector as well). Please let me know if I am not doing so.

Nirhar · 2024-08-08T13:01:07Z

ping @arsenm @phoebewang !

llvm/test/CodeGen/X86/insert-into-vector-through-stack-no-stack-realign.ll

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 could 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

RKSimon

LGTM

Nirhar · 2024-08-09T14:10:18Z

@RKSimon Can you merge on my behalf?

* 'main' of https://github.com/llvm/llvm-project: (700 commits) [SandboxIR][NFC] SingleLLVMInstructionImpl class (llvm#102687) [ThinLTO]Clean up 'import-assume-unique-local' flag. (llvm#102424) [nsan] Make #include more conventional [SandboxIR][NFC] Use Tracker.emplaceIfTracking() [libc] Moved range_reduction_double ifdef statement (llvm#102659) [libc] Fix CFP long double and add tests (llvm#102660) [TargetLowering] Handle vector types in expandFixedPointMul (llvm#102635) [compiler-rt][NFC] Replace environment variable with %t (llvm#102197) [UnitTests] Convert a test to use opaque pointers (llvm#102668) [CodeGen][NFCI] Don't re-implement parts of ASTContext::getIntWidth (llvm#101765) [SandboxIR] Clean up tracking code with the help of emplaceIfTracking() (llvm#102406) [mlir][bazel] remove extra blanks in mlir-tblgen test [NVPTX][NFC] Update tests to use bfloat type (llvm#101493) [mlir] Add support for parsing nested PassPipelineOptions (llvm#101118) [mlir][bazel] add missing td dependency in mlir-tblgen test [flang][cuda] Fix lib dependency [libc] Clean up remaining use of *_WIDTH macros in printf (llvm#102679) [flang][cuda] Convert cuf.alloc for box to fir.alloca in device context (llvm#102662) [SandboxIR] Implement the InsertElementInst class (llvm#102404) [libc] Fix use of cpp::numeric_limits<...>::digits (llvm#102674) [mlir][ODS] Verify type constraints in Types and Attributes (llvm#102326) [LTO] enable `ObjCARCContractPass` only on optimized build (llvm#101114) [mlir][ODS] Consistent `cppType` / `cppClassName` usage (llvm#102657) [lldb] Move definition of SBSaveCoreOptions dtor out of header (llvm#102539) [libc] Use cpp::numeric_limits in preference to C23 <limits.h> macros (llvm#102665) [clang] Implement -fptrauth-auth-traps. (llvm#102417) [LLVM][rtsan] rtsan transform to preserve CFGAnalyses (llvm#102651) Revert "[AMDGPU] Move `AMDGPUAttributorPass` to full LTO post link stage (llvm#102086)" [RISCV][GISel] Add missing tests for G_CTLZ/CTTZ instruction selection. NFC Return available function types for BindingDecls. (llvm#102196) [clang] Wire -fptrauth-returns to "ptrauth-returns" fn attribute. (llvm#102416) [RISCV] Remove riscv-experimental-rv64-legal-i32. (llvm#102509) [RISCV] Move PseudoVSET(I)VLI expansion to use PseudoInstExpansion. (llvm#102496) [NVPTX] support switch statement with brx.idx (reland) (llvm#102550) [libc][newhdrgen]sorted function names in yaml (llvm#102544) [GlobalIsel] Combine G_ADD and G_SUB with constants (llvm#97771) Suppress spurious warnings due to R_RISCV_SET_ULEB128 [scudo] Separated committed and decommitted entries. (llvm#101409) [MIPS] Fix missing ANDI optimization (llvm#97689) [Clang] Add env var for nvptx-arch/amdgpu-arch timeout (llvm#102521) [asan] Switch allocator to dynamic base address (llvm#98511) [AMDGPU] Move `AMDGPUAttributorPass` to full LTO post link stage (llvm#102086) [libc][math][c23] Add fadd{l,f128} C23 math functions (llvm#102531) [mlir][bazel] revert bazel rule change for DLTITransformOps [msan] Support vst{2,3,4}_lane instructions (llvm#101215) Revert "[MLIR][DLTI][Transform] Introduce transform.dlti.query (llvm#101561)" [X86] pr57673.ll - generate MIR test checks [mlir][vector][test] Split tests from vector-transfer-flatten.mlir (llvm#102584) [mlir][bazel] add bazel rule for DLTITransformOps OpenMPOpt: Remove dead include [IR] Add method to GlobalVariable to change type of initializer. (llvm#102553) [flang][cuda] Force default allocator in device code (llvm#102238) [llvm] Construct SmallVector<SDValue> with ArrayRef (NFC) (llvm#102578) [MLIR][DLTI][Transform] Introduce transform.dlti.query (llvm#101561) [AMDGPU][AsmParser][NFC] Remove a misleading comment. (llvm#102604) [Arm][AArch64][Clang] Respect function's branch protection attributes. (llvm#101978) [mlir] Verifier: steal bit to track seen instead of set. (llvm#102626) [Clang] Fix Handling of Init Capture with Parameter Packs in LambdaScopeForCallOperatorInstantiationRAII (llvm#100766) [X86] Convert truncsat clamping patterns to use SDPatternMatch. NFC. [gn] Give two scripts argparse.RawDescriptionHelpFormatter [bazel] Add missing dep for the SPIRVToLLVM target [Clang] Simplify specifying passes via -Xoffload-linker (llvm#102483) [bazel] Port for d45de80 [SelectionDAG] Use unaligned store/load to move AVX registers onto stack for `insertelement` (llvm#82130) [Clang][OMPX] Add the code generation for multi-dim `num_teams` (llvm#101407) [ARM] Regenerate big-endian-vmov.ll. NFC [AMDGPU][AsmParser][NFCI] All NamedIntOperands to be of the i32 type. (llvm#102616) [libc][math][c23] Add totalorderl function. (llvm#102564) [mlir][spirv] Support `memref` in `convert-to-spirv` pass (llvm#102534) [MLIR][GPU-LLVM] Convert `gpu.func` to `llvm.func` (llvm#101664) Fix a unit test input file (llvm#102567) [llvm-readobj][COFF] Dump hybrid objects for ARM64X files. (llvm#102245) AMDGPU/NewPM: Port SIFixSGPRCopies to new pass manager (llvm#102614) [MemoryBuiltins] Simplify getCalledFunction() helper (NFC) [AArch64] Add invalid 1 x vscale costs for reductions and reduction-operations. (llvm#102105) [MemoryBuiltins] Handle allocator attributes on call-site LSV/test/AArch64: add missing lit.local.cfg; fix build (llvm#102607) Revert "Enable logf128 constant folding for hosts with 128bit floats (llvm#96287)" [RISCV] Add Syntacore SCR5 RV32/64 processors definition (llvm#102285) [InstCombine] Remove unnecessary RUN line from test (NFC) [flang][OpenMP] Handle multiple ranges in `num_teams` clause (llvm#102535) [mlir][vector] Add tests for scalable vectors in one-shot-bufferize.mlir (llvm#102361) [mlir][vector] Disable `vector.matrix_multiply` for scalable vectors (llvm#102573) [clang] Implement CWG2627 Bit-fields and narrowing conversions (llvm#78112) [NFC] Use references to avoid copying (llvm#99863) Revert "[mlir][ArmSME] Pattern to swap shape_cast(tranpose) with transpose(shape_cast) (llvm#100731)" (llvm#102457) [IRBuilder] Generate nuw GEPs for struct member accesses (llvm#99538) [bazel] Port for 9b06e25 [CodeGen][NewPM] Improve start/stop pass error message CodeGenPassBuilder (llvm#102591) [AArch64] Implement TRBMPAM_EL1 system register (llvm#102485) [InstCombine] Fixing wrong select folding in vectors with undef elements (llvm#102244) [AArch64] Sink operands to fmuladd. (llvm#102297) LSV: document hang reported in llvm#37865 (llvm#102479) Enable logf128 constant folding for hosts with 128bit floats (llvm#96287) [RISCV][clang] Remove bfloat base type in non-zvfbfmin vcreate (llvm#102146) [RISCV][clang] Add missing `zvfbfmin` to `vget_v` intrinsic (llvm#102149) [mlir][vector] Add mask elimination transform (llvm#99314) [Clang][Interp] Fix display of syntactically-invalid note for member function calls (llvm#102170) [bazel] Port for 3fffa6d [DebugInfo][RemoveDIs] Use iterator-inserters in clang (llvm#102006) ... Signed-off-by: Edwiin Kusuma Jaya <kutemeikito0905@gmail.com>

llvmbot added backend:X86 llvm:SelectionDAG SelectionDAGISel as well labels Feb 17, 2024

RKSimon requested review from RKSimon, phoebewang and XinWang10 February 18, 2024 10:57

RKSimon reviewed Feb 18, 2024

View reviewed changes

llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp Outdated Show resolved Hide resolved

Nirhar force-pushed the alignment-error-2 branch from 2f0989e to 92a4d89 Compare February 20, 2024 15:21

arsenm reviewed Feb 26, 2024

View reviewed changes

arsenm reviewed Mar 5, 2024

View reviewed changes

Nirhar force-pushed the alignment-error-2 branch 2 times, most recently from 6b45121 to 8e3b74b Compare July 16, 2024 12:44

Nirhar requested review from arsenm and RKSimon July 16, 2024 12:46

annamthomas reviewed Jul 16, 2024

View reviewed changes

llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp Outdated Show resolved Hide resolved

Nirhar force-pushed the alignment-error-2 branch 2 times, most recently from ea0a99a to caca20a Compare July 22, 2024 16:40

arsenm reviewed Jul 22, 2024

View reviewed changes

llvm/test/CodeGen/X86/unaligned-insert-into-vector-through-stack.ll Outdated Show resolved Hide resolved

phoebewang mentioned this pull request Jul 27, 2024

[SelectionDAG] Use unaligned store to legalize EXTRACT_VECTOR_ELT type when Stack is non-realignable #98176

Merged

Nirhar force-pushed the alignment-error-2 branch from caca20a to 559a713 Compare July 29, 2024 15:48

arsenm reviewed Jul 29, 2024

View reviewed changes

Nirhar force-pushed the alignment-error-2 branch from 559a713 to 2500579 Compare July 31, 2024 12:35

Nirhar requested a review from arsenm July 31, 2024 14:06

arsenm reviewed Jul 31, 2024

View reviewed changes

Nirhar force-pushed the alignment-error-2 branch 3 times, most recently from 38bbfa9 to c867389 Compare August 4, 2024 16:11

Nirhar requested a review from arsenm August 5, 2024 02:59

RKSimon reviewed Aug 8, 2024

View reviewed changes

llvm/test/CodeGen/X86/insert-into-vector-through-stack-no-stack-realign.ll Outdated Show resolved Hide resolved

Nirhar force-pushed the alignment-error-2 branch from c867389 to cf64ce3 Compare August 9, 2024 06:26

RKSimon approved these changes Aug 9, 2024

View reviewed changes

RKSimon merged commit 259742a into llvm:main Aug 9, 2024
8 checks passed

		DAG.getDataLayout().getPrefTypeAlign(
		Op.getValueType().getTypeForEVT(*DAG.getContext())));

[SelectionDAG] Use unaligned store/load to move AVX registers onto stack for insertelement #82130

[SelectionDAG] Use unaligned store/load to move AVX registers onto stack for insertelement #82130

Conversation

Nirhar commented Feb 17, 2024 • edited by RKSimon Loading

llvmbot commented Feb 17, 2024 • edited Loading

Nirhar commented Feb 17, 2024

Nirhar commented Feb 17, 2024

github-actions bot commented Feb 20, 2024 • edited Loading

RKSimon commented Feb 20, 2024

Nirhar commented Feb 20, 2024

arsenm commented Feb 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RKSimon commented Apr 2, 2024

Nirhar commented Jul 22, 2024

arsenm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nirhar Jul 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nirhar commented Jul 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nirhar Aug 4, 2024 • edited Loading

Choose a reason for hiding this comment

Nirhar commented Aug 8, 2024

RKSimon left a comment

Choose a reason for hiding this comment

Nirhar commented Aug 9, 2024

[SelectionDAG] Use unaligned store/load to move AVX registers onto stack for `insertelement` #82130

[SelectionDAG] Use unaligned store/load to move AVX registers onto stack for `insertelement` #82130

Nirhar commented Feb 17, 2024 •

edited by RKSimon

Loading

llvmbot commented Feb 17, 2024 •

edited

Loading

github-actions bot commented Feb 20, 2024 •

edited

Loading

Nirhar Jul 26, 2024 •

edited

Loading

Nirhar Aug 4, 2024 •

edited

Loading