[InstCombine]PtrReplacer: Correctly handle select with unavailable operands #148829

Pierre-vh · 2025-07-15T11:28:54Z

The testcase I added previously failed because a SelectInst with invalid operands was created (one side addrspace(4), the other addrspace(5)).
PointerReplacer needs to dig deeper if the true and/or false instructions of the select are not available.

Fixes SWDEV-542957

…erands The testcase I added previously failed because a SelectInst with invalid operands was created (one side `addrspace(4)`, the other `addrspace(5)`). PointerReplacer needs to dig deeper if the true and/or false instructions of the select are not available. Fixes SWDEV-542957

Pierre-vh · 2025-07-15T11:29:09Z

[InstCombine]PtrReplacer: Correctly handle select with unavailable operands #148829 👈 (View in Graphite)
main

This stack of pull requests is managed by Graphite. Learn more about stacking.

llvmbot · 2025-07-15T11:29:48Z

@llvm/pr-subscribers-llvm-transforms

Author: Pierre van Houtryve (Pierre-vh)

Changes

The testcase I added previously failed because a SelectInst with invalid operands was created (one side addrspace(4), the other addrspace(5)).
PointerReplacer needs to dig deeper if the true and/or false instructions of the select are not available.

Fixes SWDEV-542957

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

2 Files Affected:

(modified) llvm/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp (+21-7)
(added) llvm/test/Transforms/InstCombine/AMDGPU/ptr-replacer-select-addrspacecast-crash.ll (+20)

diff --git a/llvm/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp b/llvm/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
index e791bc4b56e52..2cc1bc9fa4a67 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
@@ -277,6 +277,15 @@ bool PointerReplacer::collectUsers() {
           Worklist.emplace_back(I);
   };
 
+  auto TryPushInstOperand = [&](Instruction *InstOp) {
+    if (!UsersToReplace.contains(InstOp)) {
+      if (!ValuesToRevisit.insert(InstOp))
+        return false;
+      Worklist.emplace_back(InstOp);
+    }
+    return true;
+  };
+
   PushUsersToWorklist(&Root);
   while (!Worklist.empty()) {
     Instruction *Inst = Worklist.pop_back_val();
@@ -309,12 +318,8 @@ bool PointerReplacer::collectUsers() {
       // incoming values.
       Worklist.emplace_back(PHI);
       for (unsigned Idx = 0; Idx < PHI->getNumIncomingValues(); ++Idx) {
-        auto *IncomingValue = cast<Instruction>(PHI->getIncomingValue(Idx));
-        if (UsersToReplace.contains(IncomingValue))
-          continue;
-        if (!ValuesToRevisit.insert(IncomingValue))
+        if (!TryPushInstOperand(cast<Instruction>(PHI->getIncomingValue(Idx))))
           return false;
-        Worklist.emplace_back(IncomingValue);
       }
     } else if (auto *SI = dyn_cast<SelectInst>(Inst)) {
       auto *TrueInst = dyn_cast<Instruction>(SI->getTrueValue());
@@ -322,8 +327,17 @@ bool PointerReplacer::collectUsers() {
       if (!TrueInst || !FalseInst)
         return false;
 
-      UsersToReplace.insert(SI);
-      PushUsersToWorklist(SI);
+      if (isAvailable(TrueInst) && isAvailable(FalseInst)) {
+        UsersToReplace.insert(SI);
+        PushUsersToWorklist(SI);
+        continue;
+      }
+
+      // Push select back onto the stack, followed by unavailable true/false
+      // value.
+      Worklist.emplace_back(SI);
+      if (!TryPushInstOperand(TrueInst) || !TryPushInstOperand(FalseInst))
+        return false;
     } else if (auto *GEP = dyn_cast<GetElementPtrInst>(Inst)) {
       UsersToReplace.insert(GEP);
       PushUsersToWorklist(GEP);
diff --git a/llvm/test/Transforms/InstCombine/AMDGPU/ptr-replacer-select-addrspacecast-crash.ll b/llvm/test/Transforms/InstCombine/AMDGPU/ptr-replacer-select-addrspacecast-crash.ll
new file mode 100644
index 0000000000000..921a72364e6f9
--- /dev/null
+++ b/llvm/test/Transforms/InstCombine/AMDGPU/ptr-replacer-select-addrspacecast-crash.ll
@@ -0,0 +1,20 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 5
+; RUN: opt -mtriple=amdgcn-amd-amdhsa -passes=instcombine %s -S -o - | FileCheck %s
+
+; Crashed in IC PtrReplacer because an invalid select was generated with addrspace(4) and addrspace(5)
+; operands.
+
+define amdgpu_kernel void @eggs(ptr addrspace(4) byref([12 x i8]) align 16 %arg) {
+; CHECK-LABEL: define amdgpu_kernel void @eggs(
+; CHECK-SAME: ptr addrspace(4) byref([12 x i8]) align 16 [[ARG:%.*]]) {
+; CHECK-NEXT:  [[BB:.*:]]
+; CHECK-NEXT:    ret void
+;
+bb:
+  %alloca = alloca i32, i32 0, align 8, addrspace(5)
+  %alloca1 = alloca [12 x i8], align 16, addrspace(5)
+  call void @llvm.memcpy.p5.p4.i64(ptr addrspace(5) %alloca1, ptr addrspace(4) %arg, i64 0, i1 false)
+  %select = select i1 false, ptr addrspace(5) %alloca1, ptr addrspace(5) %alloca
+  call void @llvm.memcpy.p0.p5.i64(ptr null, ptr addrspace(5) %select, i64 0, i1 false)
+  ret void
+}

llvmbot · 2025-07-15T11:29:50Z

@llvm/pr-subscribers-backend-amdgpu

Author: Pierre van Houtryve (Pierre-vh)

Changes

The testcase I added previously failed because a SelectInst with invalid operands was created (one side addrspace(4), the other addrspace(5)).
PointerReplacer needs to dig deeper if the true and/or false instructions of the select are not available.

Fixes SWDEV-542957

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

2 Files Affected:

(modified) llvm/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp (+21-7)
(added) llvm/test/Transforms/InstCombine/AMDGPU/ptr-replacer-select-addrspacecast-crash.ll (+20)

diff --git a/llvm/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp b/llvm/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
index e791bc4b56e52..2cc1bc9fa4a67 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
@@ -277,6 +277,15 @@ bool PointerReplacer::collectUsers() {
           Worklist.emplace_back(I);
   };
 
+  auto TryPushInstOperand = [&](Instruction *InstOp) {
+    if (!UsersToReplace.contains(InstOp)) {
+      if (!ValuesToRevisit.insert(InstOp))
+        return false;
+      Worklist.emplace_back(InstOp);
+    }
+    return true;
+  };
+
   PushUsersToWorklist(&Root);
   while (!Worklist.empty()) {
     Instruction *Inst = Worklist.pop_back_val();
@@ -309,12 +318,8 @@ bool PointerReplacer::collectUsers() {
       // incoming values.
       Worklist.emplace_back(PHI);
       for (unsigned Idx = 0; Idx < PHI->getNumIncomingValues(); ++Idx) {
-        auto *IncomingValue = cast<Instruction>(PHI->getIncomingValue(Idx));
-        if (UsersToReplace.contains(IncomingValue))
-          continue;
-        if (!ValuesToRevisit.insert(IncomingValue))
+        if (!TryPushInstOperand(cast<Instruction>(PHI->getIncomingValue(Idx))))
           return false;
-        Worklist.emplace_back(IncomingValue);
       }
     } else if (auto *SI = dyn_cast<SelectInst>(Inst)) {
       auto *TrueInst = dyn_cast<Instruction>(SI->getTrueValue());
@@ -322,8 +327,17 @@ bool PointerReplacer::collectUsers() {
       if (!TrueInst || !FalseInst)
         return false;
 
-      UsersToReplace.insert(SI);
-      PushUsersToWorklist(SI);
+      if (isAvailable(TrueInst) && isAvailable(FalseInst)) {
+        UsersToReplace.insert(SI);
+        PushUsersToWorklist(SI);
+        continue;
+      }
+
+      // Push select back onto the stack, followed by unavailable true/false
+      // value.
+      Worklist.emplace_back(SI);
+      if (!TryPushInstOperand(TrueInst) || !TryPushInstOperand(FalseInst))
+        return false;
     } else if (auto *GEP = dyn_cast<GetElementPtrInst>(Inst)) {
       UsersToReplace.insert(GEP);
       PushUsersToWorklist(GEP);
diff --git a/llvm/test/Transforms/InstCombine/AMDGPU/ptr-replacer-select-addrspacecast-crash.ll b/llvm/test/Transforms/InstCombine/AMDGPU/ptr-replacer-select-addrspacecast-crash.ll
new file mode 100644
index 0000000000000..921a72364e6f9
--- /dev/null
+++ b/llvm/test/Transforms/InstCombine/AMDGPU/ptr-replacer-select-addrspacecast-crash.ll
@@ -0,0 +1,20 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 5
+; RUN: opt -mtriple=amdgcn-amd-amdhsa -passes=instcombine %s -S -o - | FileCheck %s
+
+; Crashed in IC PtrReplacer because an invalid select was generated with addrspace(4) and addrspace(5)
+; operands.
+
+define amdgpu_kernel void @eggs(ptr addrspace(4) byref([12 x i8]) align 16 %arg) {
+; CHECK-LABEL: define amdgpu_kernel void @eggs(
+; CHECK-SAME: ptr addrspace(4) byref([12 x i8]) align 16 [[ARG:%.*]]) {
+; CHECK-NEXT:  [[BB:.*:]]
+; CHECK-NEXT:    ret void
+;
+bb:
+  %alloca = alloca i32, i32 0, align 8, addrspace(5)
+  %alloca1 = alloca [12 x i8], align 16, addrspace(5)
+  call void @llvm.memcpy.p5.p4.i64(ptr addrspace(5) %alloca1, ptr addrspace(4) %arg, i64 0, i1 false)
+  %select = select i1 false, ptr addrspace(5) %alloca1, ptr addrspace(5) %alloca
+  call void @llvm.memcpy.p0.p5.i64(ptr null, ptr addrspace(5) %select, i64 0, i1 false)
+  ret void
+}

llvm/test/Transforms/InstCombine/AMDGPU/ptr-replacer-select-addrspacecast-crash.ll

shiltian · 2025-07-15T13:55:24Z

llvm/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp

-        if (UsersToReplace.contains(IncomingValue))
-          continue;
-        if (!ValuesToRevisit.insert(IncomingValue))
+        if (!TryPushInstOperand(cast<Instruction>(PHI->getIncomingValue(Idx))))


This is not tested

This is a NFC and is already being tested in AMDGPU/ptr-replace-alloca.ll

gandhi56 · 2025-07-15T17:02:49Z

llvm/test/Transforms/InstCombine/AMDGPU/ptr-replacer-select-addrspacecast-crash.ll

+; Crashed in IC PtrReplacer because an invalid select was generated with addrspace(4) and addrspace(5)
+; operands.
+
+define amdgpu_kernel void @eggs(ptr addrspace(4) byref([12 x i8]) align 16 %arg) {


More descriptive function name, please.

eggs and milk, I actually love it. :-D

Sure, I don't mind as long as other tests are named bagel, croissant, etc. Consistent breakfast is key for a healthy compiler ;)

no I was just kidding :-D

In my defense, the first one came from -passes=metarenamer (which I always use on reduced test cases), but the second one is just me continuing the trend 😄

I'll update the test names

gandhi56 · 2025-07-15T17:03:40Z

llvm/test/Transforms/InstCombine/AMDGPU/ptr-replacer-select-addrspacecast-crash.ll

+  ret void
+}
+
+define amdgpu_kernel void @milk(ptr addrspace(4) byref([12 x i8]) align 16 %arg) {


gandhi56 · 2025-07-15T17:04:23Z

llvm/test/Transforms/InstCombine/AMDGPU/ptr-replacer-select-addrspacecast-crash.ll

These tests could be merged into AMDGPU/ptr-replace-alloca.ll

llvm-ci · 2025-07-16T07:38:47Z

LLVM Buildbot has detected a new failure on builder lldb-x86_64-debian running on lldb-x86_64-debian while building llvm at step 6 "test".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/162/builds/26944

Here is the relevant piece of the build log for the reference

Step 6 (test) failure: build (failure)
...
UNSUPPORTED: lldb-shell :: ScriptInterpreter/Lua/independent_state.test (3081 of 3092)
UNSUPPORTED: lldb-shell :: SymbolFile/DWARF/TestDedupWarnings.test (3082 of 3092)
UNSUPPORTED: lldb-shell :: ScriptInterpreter/Python/Crashlog/altered_threadState.test (3083 of 3092)
UNSUPPORTED: lldb-shell :: ScriptInterpreter/Python/Crashlog/no_threadState.test (3084 of 3092)
UNSUPPORTED: lldb-shell :: ScriptInterpreter/Lua/persistent_state.test (3085 of 3092)
UNSUPPORTED: lldb-shell :: Watchpoint/LocalVariableWatchpointDisabler.test (3086 of 3092)
UNSUPPORTED: lldb-shell :: Register/x86-gp-write.test (3087 of 3092)
UNSUPPORTED: lldb-shell :: ScriptInterpreter/Python/Crashlog/parser_json.test (3088 of 3092)
PASS: lldb-api :: terminal/TestEditlineCompletions.py (3089 of 3092)
UNRESOLVED: lldb-api :: tools/lldb-dap/launch/TestDAP_launch.py (3090 of 3092)
******************** TEST 'lldb-api :: tools/lldb-dap/launch/TestDAP_launch.py' FAILED ********************
Script:
--
/usr/bin/python3 /home/worker/2.0.1/lldb-x86_64-debian/llvm-project/lldb/test/API/dotest.py -u CXXFLAGS -u CFLAGS --env LLVM_LIBS_DIR=/home/worker/2.0.1/lldb-x86_64-debian/build/./lib --env LLVM_INCLUDE_DIR=/home/worker/2.0.1/lldb-x86_64-debian/build/include --env LLVM_TOOLS_DIR=/home/worker/2.0.1/lldb-x86_64-debian/build/./bin --arch x86_64 --build-dir /home/worker/2.0.1/lldb-x86_64-debian/build/lldb-test-build.noindex --lldb-module-cache-dir /home/worker/2.0.1/lldb-x86_64-debian/build/lldb-test-build.noindex/module-cache-lldb/lldb-api --clang-module-cache-dir /home/worker/2.0.1/lldb-x86_64-debian/build/lldb-test-build.noindex/module-cache-clang/lldb-api --executable /home/worker/2.0.1/lldb-x86_64-debian/build/./bin/lldb --compiler /home/worker/2.0.1/lldb-x86_64-debian/build/./bin/clang --dsymutil /home/worker/2.0.1/lldb-x86_64-debian/build/./bin/dsymutil --make /usr/bin/gmake --llvm-tools-dir /home/worker/2.0.1/lldb-x86_64-debian/build/./bin --lldb-obj-root /home/worker/2.0.1/lldb-x86_64-debian/build/tools/lldb --lldb-libs-dir /home/worker/2.0.1/lldb-x86_64-debian/build/./lib --cmake-build-type Release -t /home/worker/2.0.1/lldb-x86_64-debian/llvm-project/lldb/test/API/tools/lldb-dap/launch -p TestDAP_launch.py
--
Exit Code: 1

Command Output (stdout):
--
lldb version 22.0.0git (https://github.com/llvm/llvm-project.git revision f223411e2e005ecd54523801db603b54d69f3159)
  clang revision f223411e2e005ecd54523801db603b54d69f3159
  llvm revision f223411e2e005ecd54523801db603b54d69f3159
Skipping the following test categories: ['libc++', 'msvcstl', 'dsym', 'gmodules', 'debugserver', 'objc']

--
Command Output (stderr):
--
Change dir to: /home/worker/2.0.1/lldb-x86_64-debian/llvm-project/lldb/test/API/tools/lldb-dap/launch
runCmd: settings clear --all

output: 

runCmd: settings set symbols.enable-external-lookup false

output: 

runCmd: settings set target.inherit-tcc true

output: 

runCmd: settings set target.disable-aslr false

output: 

runCmd: settings set target.detach-on-error false

output: 

runCmd: settings set target.auto-apply-fixits false

Pierre-vh requested review from arsenm, gandhi56 and shiltian July 15, 2025 11:29

Pierre-vh marked this pull request as ready for review July 15, 2025 11:29

Pierre-vh requested a review from nikic as a code owner July 15, 2025 11:29

llvmbot added backend:AMDGPU llvm:instcombine Covers the InstCombine, InstSimplify and AggressiveInstCombine passes llvm:transforms labels Jul 15, 2025

arsenm reviewed Jul 15, 2025

View reviewed changes

llvm/test/Transforms/InstCombine/AMDGPU/ptr-replacer-select-addrspacecast-crash.ll Outdated Show resolved Hide resolved

llvm/test/Transforms/InstCombine/AMDGPU/ptr-replacer-select-addrspacecast-crash.ll Outdated Show resolved Hide resolved

test nits

29cb1e3

Pierre-vh requested a review from arsenm July 15, 2025 12:27

shiltian reviewed Jul 15, 2025

View reviewed changes

gandhi56 reviewed Jul 15, 2025

View reviewed changes

gandhi56 approved these changes Jul 15, 2025

View reviewed changes

update test names, move them to ptr-replace-alloca.ll

0a1bb70

Pierre-vh merged commit f223411 into main Jul 16, 2025
7 of 9 checks passed

Pierre-vh deleted the users/pierre-vh/fix-select-ptrreplace branch July 16, 2025 07:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[InstCombine]PtrReplacer: Correctly handle select with unavailable operands #148829

[InstCombine]PtrReplacer: Correctly handle select with unavailable operands #148829

Uh oh!

Pierre-vh commented Jul 15, 2025

Uh oh!

Pierre-vh commented Jul 15, 2025

Uh oh!

llvmbot commented Jul 15, 2025

Uh oh!

llvmbot commented Jul 15, 2025

Uh oh!

Uh oh!

Uh oh!

shiltian Jul 15, 2025

Uh oh!

gandhi56 Jul 15, 2025

Uh oh!

gandhi56 Jul 15, 2025

Uh oh!

shiltian Jul 15, 2025

Uh oh!

gandhi56 Jul 15, 2025

Uh oh!

shiltian Jul 15, 2025

Uh oh!

Pierre-vh Jul 16, 2025

Uh oh!

gandhi56 Jul 15, 2025

Uh oh!

gandhi56 Jul 15, 2025

Uh oh!

Uh oh!

llvm-ci commented Jul 16, 2025

Uh oh!

Uh oh!

[InstCombine]PtrReplacer: Correctly handle select with unavailable operands #148829

[InstCombine]PtrReplacer: Correctly handle select with unavailable operands #148829

Uh oh!

Conversation

Pierre-vh commented Jul 15, 2025

Uh oh!

Pierre-vh commented Jul 15, 2025

Uh oh!

llvmbot commented Jul 15, 2025

Uh oh!

llvmbot commented Jul 15, 2025

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

llvm-ci commented Jul 16, 2025

Uh oh!

Uh oh!