[SelectionDAG] Mark frame index as "aliased" at argument copy elison #89712

bjope · 2024-04-23T07:26:41Z

This is a fix for miscompiles reported in
#89060

After argument copy elison the IR value for the eliminated alloca
is aliasing with the fixed stack object. This patch is making sure
that we mark the fixed stack object as being aliased with IR values
to avoid that for example schedulers are reordering accesses to
the fixed stack object. This could otherwise happen when there is a
mix of MemOperands refering the shared fixed stack slow via both
the IR value for the elided alloca, and via a fixed stack pseudo
source value (as would be the case when lowering the arguments).

llvmbot · 2024-04-23T07:27:16Z

@llvm/pr-subscribers-llvm-selectiondag

Author: Björn Pettersson (bjope)

Changes

This is a fix for miscompiles reported in
#89060

After argument copy elison the IR value for the eliminated alloca
is aliasing with the fixed stack object. This patch is making sure
that we mark the fixed stack object as being aliased with IR values
to avoid that for example schedulers are reordering accesses to
the fixed stack object. This could otherwise happen when there is a
mix of MemOperands refering the shared fixed stack slow via both
the IR value for the elided alloca, and via a fixed stack pseudo
source value (as would be the case when lowering the arguments).

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

3 Files Affected:

(modified) llvm/include/llvm/CodeGen/MachineFrameInfo.h (+7)
(modified) llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp (+2-1)
(added) llvm/test/CodeGen/Hexagon/arg-copy-elison.ll (+39)

diff --git a/llvm/include/llvm/CodeGen/MachineFrameInfo.h b/llvm/include/llvm/CodeGen/MachineFrameInfo.h
index 0fe73fec7ee67f..a2c78e9e093d01 100644
--- a/llvm/include/llvm/CodeGen/MachineFrameInfo.h
+++ b/llvm/include/llvm/CodeGen/MachineFrameInfo.h
@@ -701,6 +701,13 @@ class MachineFrameInfo {
     return Objects[ObjectIdx+NumFixedObjects].isAliased;
   }
 
+  /// Set "maybe pointed to by an LLVM IR value" for an object.
+  void setIsAliasedObjectIndex(int ObjectIdx, bool IsAliased) {
+    assert(unsigned(ObjectIdx+NumFixedObjects) < Objects.size() &&
+           "Invalid Object Idx!");
+    Objects[ObjectIdx+NumFixedObjects].isAliased = IsAliased;
+  }
+
   /// Returns true if the specified index corresponds to an immutable object.
   bool isImmutableObjectIndex(int ObjectIdx) const {
     // Tail calling functions can clobber their function arguments.
diff --git a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
index 3f69f7ad54477e..319465865fb1d1 100644
--- a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
@@ -11128,7 +11128,7 @@ static void tryToElideArgumentCopy(
   }
 
   // Perform the elision. Delete the old stack object and replace its only use
-  // in the variable info map. Mark the stack object as mutable.
+  // in the variable info map. Mark the stack object as mutable and aliased.
   LLVM_DEBUG({
     dbgs() << "Eliding argument copy from " << Arg << " to " << *AI << '\n'
            << "  Replacing frame index " << OldIndex << " with " << FixedIndex
@@ -11136,6 +11136,7 @@ static void tryToElideArgumentCopy(
   });
   MFI.RemoveStackObject(OldIndex);
   MFI.setIsImmutableObjectIndex(FixedIndex, false);
+  MFI.setIsAliasedObjectIndex(FixedIndex, true);
   AllocaIndex = FixedIndex;
   ArgCopyElisionFrameIndexMap.insert({OldIndex, FixedIndex});
   for (SDValue ArgVal : ArgVals)
diff --git a/llvm/test/CodeGen/Hexagon/arg-copy-elison.ll b/llvm/test/CodeGen/Hexagon/arg-copy-elison.ll
new file mode 100644
index 00000000000000..f7902e65c46953
--- /dev/null
+++ b/llvm/test/CodeGen/Hexagon/arg-copy-elison.ll
@@ -0,0 +1,39 @@
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py UTC_ARGS: --version 4
+; RUN: llc -mtriple hexagon-- -O3 -o - %s | FileCheck %s
+
+; Reproducer for https://github.com/llvm/llvm-project/issues/89060
+;
+; Problem was a bug in argument copy elison. Given that the %alloca is
+; eliminated, the same frame index will be used for accessing %alloca and %a
+; on the fixed stack. Care must be taken when setting up
+; MachinePointerInfo/MemOperands for those accesses to either make sure that
+; we always refer to the fixed stack slot the same way (not using the
+; ir.alloca name), or make sure that we still detect that they alias each
+; other if using different kinds of MemOperands to identify the same fixed
+; stack entry.
+;
+define i32 @f(i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32 %q1, i32 %a, i32 %q2) {
+; CHECK-LABEL: f:
+; CHECK:         .cfi_startproc
+; CHECK-NEXT:  // %bb.0:
+; CHECK-NEXT:    {
+; CHECK-NEXT:     r0 = memw(r29+#36)
+; CHECK-NEXT:     r1 = memw(r29+#28)
+; CHECK-NEXT:    }
+; CHECK-NEXT:    {
+; CHECK-NEXT:     r0 = sub(r1,r0)
+; CHECK-NEXT:     r2 = memw(r29+#32)
+; CHECK-NEXT:     memw(r29+#32) = ##666
+; CHECK-NEXT:    }
+; CHECK-NEXT:    {
+; CHECK-NEXT:     r0 = xor(r0,r2)
+; CHECK-NEXT:     jumpr r31
+; CHECK-NEXT:    }
+  %alloca = alloca i32
+  store i32 %a, ptr %alloca     ; Should be elided.
+  store i32 666, ptr %alloca
+  %x = sub i32 %q1, %q2
+  %y = xor i32 %x, %a           ; Results in a load of %a from fixed stack.
+                                ; Using same frame index as elided %alloca.
+  ret i32 %y
+}

github-actions · 2024-04-23T07:29:34Z

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff 56ed3dd77f4ef07a9b35cbb3d0ca868cc0999d01 5d7f7b4c33ef69af1348d1265882359d6cc1d853 -- llvm/include/llvm/CodeGen/MachineFrameInfo.h llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

View the diff from clang-format here.

diff --git a/llvm/include/llvm/CodeGen/MachineFrameInfo.h b/llvm/include/llvm/CodeGen/MachineFrameInfo.h
index a2c78e9e09..8c60c0d091 100644
--- a/llvm/include/llvm/CodeGen/MachineFrameInfo.h
+++ b/llvm/include/llvm/CodeGen/MachineFrameInfo.h
@@ -703,9 +703,9 @@ public:
 
   /// Set "maybe pointed to by an LLVM IR value" for an object.
   void setIsAliasedObjectIndex(int ObjectIdx, bool IsAliased) {
-    assert(unsigned(ObjectIdx+NumFixedObjects) < Objects.size() &&
+    assert(unsigned(ObjectIdx + NumFixedObjects) < Objects.size() &&
            "Invalid Object Idx!");
-    Objects[ObjectIdx+NumFixedObjects].isAliased = IsAliased;
+    Objects[ObjectIdx + NumFixedObjects].isAliased = IsAliased;
   }
 
   /// Returns true if the specified index corresponds to an immutable object.

arsenm · 2024-04-23T08:44:42Z

llvm/test/CodeGen/Hexagon/arg-copy-elison.ll

@@ -0,0 +1,39 @@
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py UTC_ARGS: --version 4
+; RUN: llc -mtriple hexagon-- -O3 -o - %s | FileCheck %s


Do you really need the -O3? It doesn't make a difference on most targets and -O2 is the default

Looks like -O2 also reproduce the failure (while -O1 won't trigger the problem). I'll change it to -O2.

Just drop the flag

arsenm · 2024-04-23T08:45:42Z

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

@@ -11128,14 +11128,15 @@ static void tryToElideArgumentCopy(
  }

  // Perform the elision. Delete the old stack object and replace its only use
-  // in the variable info map. Mark the stack object as mutable.
+  // in the variable info map. Mark the stack object as mutable and aliased.


Is there any practical difference between the mutable and isAliased cases? Where is the ultimate alias query that goes wrong?

isAliased is used inside ScheduleDAGInstrs.cpp getUnderlyingObjectsForInstr:

if (const PseudoSourceValue *PSV = MMO->getPseudoValue()) { // Function that contain tail calls don't have unique PseudoSourceValue // objects. Two PseudoSourceValues might refer to the same or // overlapping locations. The client code calling this function assumes // this is not the case. So return a conservative answer of no known // object. if (MFI.hasTailCall()) return false; // For now, ignore PseudoSourceValues which may alias LLVM IR values // because the code that uses this function has no way to cope with // such aliases. if (PSV->isAliased(&MFI)) return false; bool MayAlias = PSV->mayAlias(&MFI); Objects.emplace_back(PSV, MayAlias); } else if (const Value *V = MMO->getValue()) {

arsenm · 2024-04-23T09:28:13Z

llvm/include/llvm/CodeGen/MachineFrameInfo.h

@@ -701,6 +701,13 @@ class MachineFrameInfo {
    return Objects[ObjectIdx+NumFixedObjects].isAliased;
  }

+  /// Set "maybe pointed to by an LLVM IR value" for an object.
+  void setIsAliasedObjectIndex(int ObjectIdx, bool IsAliased) {


Suggested change

void setIsAliasedObjectIndex(int ObjectIdx, bool IsAliased) {

void setIsAliasedObjectIndex(int ObjectIdx, bool IsAliased = true) {

arsenm · 2024-04-23T09:28:23Z

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

  LLVM_DEBUG({
    dbgs() << "Eliding argument copy from " << Arg << " to " << *AI << '\n'
           << "  Replacing frame index " << OldIndex << " with " << FixedIndex
           << '\n';
  });
  MFI.RemoveStackObject(OldIndex);
  MFI.setIsImmutableObjectIndex(FixedIndex, false);
+  MFI.setIsAliasedObjectIndex(FixedIndex, true);


Suggested change

MFI.setIsAliasedObjectIndex(FixedIndex, true);

MFI.setIsAliasedObjectIndex(FixedIndex);

arsenm · 2024-04-23T09:28:41Z

llvm/test/CodeGen/Hexagon/arg-copy-elison.ll

@@ -0,0 +1,39 @@
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py UTC_ARGS: --version 4
+; RUN: llc -mtriple hexagon-- -O3 -o - %s | FileCheck %s


Just drop the flag

This is a fix for miscompiles reported in llvm#89060 After argument copy elison the IR value for the eliminated alloca is aliasing with the fixed stack object. This patch is making sure that we mark the fixed stack object as being aliased with IR values to avoid that for example schedulers are reordering accesses to the fixed stack object. This could otherwise happen when there is a mix of MemOperands refering the shared fixed stack slow via both the IR value for the elided alloca, and via a fixed stack pseudo source value (as would be the case when lowering the arguments).

AZero13 · 2024-04-23T16:56:50Z

/cherry-pick d8b253b

llvmbot · 2024-04-23T17:02:13Z

/cherry-pick d8b253b

Error: Command failed due to missing milestone.

…lvm#89712) This is a fix for miscompiles reported in llvm#89060 After argument copy elison the IR value for the eliminated alloca is aliasing with the fixed stack object. This patch is making sure that we mark the fixed stack object as being aliased with IR values to avoid that for example schedulers are reordering accesses to the fixed stack object. This could otherwise happen when there is a mix of MemOperands refering the shared fixed stack slow via both the IR value for the elided alloca, and via a fixed stack pseudo source value (as would be the case when lowering the arguments). (cherry picked from commit d8b253b)

bjope requested review from rnk and SundeepKushwaha April 23, 2024 07:26

llvmbot added the llvm:SelectionDAG SelectionDAGISel as well label Apr 23, 2024

arsenm reviewed Apr 23, 2024

View reviewed changes

arsenm approved these changes Apr 23, 2024

View reviewed changes

bjope force-pushed the argcopyelison branch from 70a9448 to 5d7f7b4 Compare April 23, 2024 11:45

bjope merged commit d8b253b into llvm:main Apr 23, 2024
2 of 4 checks passed

aemerson mentioned this pull request May 9, 2024

release/18.x: [AArc64][GlobalISel] Fix legalizer assert for G_INSERT_VECTOR_ELT - manual merge #91672

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SelectionDAG] Mark frame index as "aliased" at argument copy elison #89712

[SelectionDAG] Mark frame index as "aliased" at argument copy elison #89712

bjope commented Apr 23, 2024

llvmbot commented Apr 23, 2024

github-actions bot commented Apr 23, 2024 •

edited

Loading

arsenm Apr 23, 2024

bjope Apr 23, 2024

arsenm Apr 23, 2024

arsenm Apr 23, 2024

bjope Apr 23, 2024

arsenm Apr 23, 2024

arsenm Apr 23, 2024

arsenm Apr 23, 2024

AZero13 commented Apr 23, 2024

llvmbot commented Apr 23, 2024

		@@ -0,0 +1,39 @@
		; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py UTC_ARGS: --version 4
		; RUN: llc -mtriple hexagon-- -O3 -o - %s \| FileCheck %s

	void setIsAliasedObjectIndex(int ObjectIdx, bool IsAliased) {
	void setIsAliasedObjectIndex(int ObjectIdx, bool IsAliased = true) {

	MFI.setIsAliasedObjectIndex(FixedIndex, true);
	MFI.setIsAliasedObjectIndex(FixedIndex);

[SelectionDAG] Mark frame index as "aliased" at argument copy elison #89712

[SelectionDAG] Mark frame index as "aliased" at argument copy elison #89712

Conversation

bjope commented Apr 23, 2024

llvmbot commented Apr 23, 2024

github-actions bot commented Apr 23, 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

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

AZero13 commented Apr 23, 2024

llvmbot commented Apr 23, 2024

github-actions bot commented Apr 23, 2024 •

edited

Loading