[GlobalISel] Preserve volatile and atomic undef/poison stores

[jlebar]

[GlobalISel] Preserve volatile and ordered atomic undef stores

It's not correct to elide a volatile or ordered atomic store, even if the
stored value is undef. You have to store something.

Fixing this exposes an issue in the AMDGPU backend. If you addrspacecast a
pointer to an illegal address space, we fold the pointer to poison. If you
then try to store such a pointer, previously we would elide the store, but now
we sometimes keep it. (Note, the pointer in the illegal address space here is
the value being stored, not the address that we're storing to.)

On AMDGPU, this causes backend crashes. There are (apparently) tests
that store such values, and the backend wasn't able to lower these "new"
instructions. We teach the backend to handle these stores by casting
the pointer to an integer first.

This bug was found by a large run of Opus 4.7 looking for bugs in LLVM.

Co-Authored-By: Claude Opus 4.8 (1M context) noreply@anthropic.com

[llvmorg-github-actions]

@llvm/pr-subscribers-backend-amdgpu

@llvm/pr-subscribers-llvm-globalisel

Author: Justin Lebar (jlebar)

Changes

It's not correct to elide a volatile or atomic store, even if the stored
value is undef.

This bug was found by a large run of Opus 4.7 looking for bugs in LLVM.

---

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

2 Files Affected:

• (modified) llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp (+4-2)
• (modified) llvm/test/CodeGen/X86/GlobalISel/undef.ll (+25)

diff --git a/llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp b/llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
index a910a900e775f..64f6a93b722ee 100644
--- a/llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
+++ b/llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
@@ -2880,8 +2880,10 @@ bool CombinerHelper::matchUndefShuffleVectorMask(MachineInstr &MI) const {
 
 bool CombinerHelper::matchUndefStore(MachineInstr &MI) const {
   assert(MI.getOpcode() == TargetOpcode::G_STORE);
-  return getOpcodeDef(TargetOpcode::G_IMPLICIT_DEF, MI.getOperand(0).getReg(),
-                      MRI);
+  auto &Store = cast<GStore>(MI);
+  if (!Store.isSimple())
+    return false;
+  return getOpcodeDef(TargetOpcode::G_IMPLICIT_DEF, Store.getValueReg(), MRI);
 }
 
 bool CombinerHelper::matchUndefSelectCmp(MachineInstr &MI) const {
diff --git a/llvm/test/CodeGen/X86/GlobalISel/undef.ll b/llvm/test/CodeGen/X86/GlobalISel/undef.ll
index 34ada8a20999d..3fd706a01944c 100644
--- a/llvm/test/CodeGen/X86/GlobalISel/undef.ll
+++ b/llvm/test/CodeGen/X86/GlobalISel/undef.ll
@@ -56,3 +56,28 @@ define float @test4(float %a) {
   ret float %r
 }
 
+define void @plain_undef_store(ptr %p) {
+; ALL-LABEL: plain_undef_store:
+; ALL:       # %bb.0:
+; ALL-NEXT:    retq
+  store i32 undef, ptr %p, align 4
+  ret void
+}
+
+define void @volatile_undef_store(ptr %p) {
+; ALL-LABEL: volatile_undef_store:
+; ALL:       # %bb.0:
+; ALL-NEXT:    movl %eax, (%rdi)
+; ALL-NEXT:    retq
+  store volatile i32 undef, ptr %p, align 4
+  ret void
+}
+
+define void @seq_cst_atomic_undef_store(ptr %p) {
+; ALL-LABEL: seq_cst_atomic_undef_store:
+; ALL:       # %bb.0:
+; ALL-NEXT:    movl %eax, (%rdi)
+; ALL-NEXT:    retq
+  store atomic i32 undef, ptr %p seq_cst, align 4
+  ret void
+}

[llvmorg-github-actions]

@llvm/pr-subscribers-backend-x86

Author: Justin Lebar (jlebar)

Changes

It's not correct to elide a volatile or atomic store, even if the stored
value is undef.

This bug was found by a large run of Opus 4.7 looking for bugs in LLVM.

---

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

2 Files Affected:

• (modified) llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp (+4-2)
• (modified) llvm/test/CodeGen/X86/GlobalISel/undef.ll (+25)

diff --git a/llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp b/llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
index a910a900e775f..64f6a93b722ee 100644
--- a/llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
+++ b/llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
@@ -2880,8 +2880,10 @@ bool CombinerHelper::matchUndefShuffleVectorMask(MachineInstr &MI) const {
 
 bool CombinerHelper::matchUndefStore(MachineInstr &MI) const {
   assert(MI.getOpcode() == TargetOpcode::G_STORE);
-  return getOpcodeDef(TargetOpcode::G_IMPLICIT_DEF, MI.getOperand(0).getReg(),
-                      MRI);
+  auto &Store = cast<GStore>(MI);
+  if (!Store.isSimple())
+    return false;
+  return getOpcodeDef(TargetOpcode::G_IMPLICIT_DEF, Store.getValueReg(), MRI);
 }
 
 bool CombinerHelper::matchUndefSelectCmp(MachineInstr &MI) const {
diff --git a/llvm/test/CodeGen/X86/GlobalISel/undef.ll b/llvm/test/CodeGen/X86/GlobalISel/undef.ll
index 34ada8a20999d..3fd706a01944c 100644
--- a/llvm/test/CodeGen/X86/GlobalISel/undef.ll
+++ b/llvm/test/CodeGen/X86/GlobalISel/undef.ll
@@ -56,3 +56,28 @@ define float @test4(float %a) {
   ret float %r
 }
 
+define void @plain_undef_store(ptr %p) {
+; ALL-LABEL: plain_undef_store:
+; ALL:       # %bb.0:
+; ALL-NEXT:    retq
+  store i32 undef, ptr %p, align 4
+  ret void
+}
+
+define void @volatile_undef_store(ptr %p) {
+; ALL-LABEL: volatile_undef_store:
+; ALL:       # %bb.0:
+; ALL-NEXT:    movl %eax, (%rdi)
+; ALL-NEXT:    retq
+  store volatile i32 undef, ptr %p, align 4
+  ret void
+}
+
+define void @seq_cst_atomic_undef_store(ptr %p) {
+; ALL-LABEL: seq_cst_atomic_undef_store:
+; ALL:       # %bb.0:
+; ALL-NEXT:    movl %eax, (%rdi)
+; ALL-NEXT:    retq
+  store atomic i32 undef, ptr %p seq_cst, align 4
+  ret void
+}

[github-actions]

⚠️ undef deprecator found issues in your code. ⚠️

You can test this locally with the following command:

git diff -U0 --pickaxe-regex -S '([^a-zA-Z0-9#_-]undef([^a-zA-Z0-9_-]|$)|UndefValue::get)' 'HEAD~1' HEAD llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp llvm/test/CodeGen/AMDGPU/invalid-addrspacecast.ll llvm/test/CodeGen/AMDGPU/lower-work-group-id-intrinsics-pal.ll llvm/test/CodeGen/X86/GlobalISel/undef.ll

The following files introduce new uses of undef:

• llvm/test/CodeGen/X86/GlobalISel/undef.ll

Undef is now deprecated and should only be used in the rare cases where no replacement is possible. For example, a load of uninitialized memory yields undef. You should use poison values for placeholders instead.

In tests, avoid using undef and having tests that trigger undefined behavior. If you need an operand with some unimportant value, you can add a new argument to the function and use that instead.

For example, this is considered a bad practice:

define void @fn() {
  ...
  br i1 undef, ...
}

Please use the following instead:

define void @fn(i1 %cond) {
  ...
  br i1 %cond, ...
}

Please refer to the Undefined Behavior Manual for more information.

[jlebar]

⚠️ undef deprecator found issues in your code. ⚠️

Definitely intentional here...

[github-actions]

🐧 Linux x64 Test Results

• 195989 tests passed
• 5286 tests skipped

✅ The build succeeded and all tests passed.

[github-actions]

🪟 Windows x64 Test Results

• 135269 tests passed
• 3341 tests skipped

✅ The build succeeded and all tests passed.

[arsenm]

It's not correct to elide a volatile or atomic store

Wouldn't this only be true for

[jlebar]

It's not correct to elide a volatile or atomic store

Wouldn't this only be true for

To finish the sentence, maybe you mean it should be legal to elide unordered atomics? I think you're right, which is another problem here.

[jlebar]

To finish the sentence, maybe you mean it should be legal to elide unordered atomics? I think you're right, which is another problem here.

@arsenm, Fixed this, and rewrote the AMDGPU backend stuff. See the PR description for an explanation of why we're touching AMDGPU here.

I have no idea why tests are creating pointers in illegal address spaces and then trying to store them. (Note we're not storing to the illegal addresses. We're just storing the pointer values.) The only value you can store in this way is poison! If you prefer I could try to change the tests to use legal address spaces, then we probably wouldn't need to edit the AMDGPU backend at all.