[ReplaceConstant] Don't create instructions for the same constant multiple times in the same basic block #169141
+60
−1
Conversation
This file contains hidden or 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
…tiple times in the same basic block Fixes #167500.
Contributor
Author
This stack of pull requests is managed by Graphite. Learn more about stacking. |
shiltian
requested review from
JonChesterfield,
arsenm,
jayfoad,
nikic and
wenju-he
Member
|
@llvm/pr-subscribers-llvm-ir @llvm/pr-subscribers-backend-amdgpu Author: Shilei Tian (shiltian) ChangesFixes #167500. Full diff: https://github.com/llvm/llvm-project/pull/169141.diff 2 Files Affected:
diff --git a/llvm/lib/IR/ReplaceConstant.cpp b/llvm/lib/IR/ReplaceConstant.cpp
index b3586b45a23f2..f3d1914a8dc82 100644
--- a/llvm/lib/IR/ReplaceConstant.cpp
+++ b/llvm/lib/IR/ReplaceConstant.cpp
@@ -91,6 +91,11 @@ bool llvm::convertUsersOfConstantsToInstructions(ArrayRef<Constant *> Consts,
// Replace those expandable operands with instructions
bool Changed = false;
+ // We need to cache the instructions we've already expanded to avoid expanding
+ // the same constant multiple times in the same basic block, which is
+ // problematic when the same constant is used in a phi node multiple times.
+ DenseMap<std::pair<Constant *, BasicBlock *>, SmallVector<Instruction *, 4>>
+ ConstantToInstructionMap;
while (!InstructionWorklist.empty()) {
Instruction *I = InstructionWorklist.pop_back_val();
DebugLoc Loc = I->getDebugLoc();
@@ -105,7 +110,10 @@ bool llvm::convertUsersOfConstantsToInstructions(ArrayRef<Constant *> Consts,
if (auto *C = dyn_cast<Constant>(U.get())) {
if (ExpandableUsers.contains(C)) {
Changed = true;
- auto NewInsts = expandUser(BI, C);
+ SmallVector<Instruction *, 4> &NewInsts =
+ ConstantToInstructionMap[std::make_pair(C, BI->getParent())];
+ if (NewInsts.empty())
+ NewInsts = expandUser(BI, C);
for (auto *NI : NewInsts)
NI->setDebugLoc(Loc);
InstructionWorklist.insert_range(NewInsts);
diff --git a/llvm/test/CodeGen/AMDGPU/same-lds-variable-multiple-use-in-one-phi-node.ll b/llvm/test/CodeGen/AMDGPU/same-lds-variable-multiple-use-in-one-phi-node.ll
new file mode 100644
index 0000000000000..35a9bee03411f
--- /dev/null
+++ b/llvm/test/CodeGen/AMDGPU/same-lds-variable-multiple-use-in-one-phi-node.ll
@@ -0,0 +1,51 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 6
+; RUN: opt -S -mtriple=amdgcn-amd-amdhsa -amdgpu-lower-module-lds %s -o - | FileCheck %s
+; RUN: opt -S -mtriple=amdgcn-amd-amdhsa -passes=amdgpu-lower-module-lds %s -o - | FileCheck %s
+
+@lds = internal unnamed_addr addrspace(3) global [6144 x half] poison, align 2
+
+define amdgpu_kernel void @test(ptr addrspace(1) %out) {
+; CHECK-LABEL: define amdgpu_kernel void @test(
+; CHECK-SAME: ptr addrspace(1) [[OUT:%.*]]) #[[ATTR0:[0-9]+]] {
+; CHECK-NEXT: [[ENTRY:.*]]:
+; CHECK-NEXT: switch i32 0, label %[[BB_3:.*]] [
+; CHECK-NEXT: i32 18, label %[[BB_2:.*]]
+; CHECK-NEXT: i32 1, label %[[BB_2]]
+; CHECK-NEXT: i32 0, label %[[BB_3]]
+; CHECK-NEXT: ]
+; CHECK: [[BB_1:.*]]:
+; CHECK-NEXT: [[TMP0:%.*]] = addrspacecast ptr addrspace(3) @llvm.amdgcn.kernel.test.lds to ptr
+; CHECK-NEXT: [[TMP1:%.*]] = ptrtoint ptr [[TMP0]] to i64
+; CHECK-NEXT: switch i32 0, label %[[BB_3]] [
+; CHECK-NEXT: i32 18, label %[[BB_2]]
+; CHECK-NEXT: i32 1, label %[[BB_2]]
+; CHECK-NEXT: i32 0, label %[[BB_3]]
+; CHECK-NEXT: ]
+; CHECK: [[BB_2]]:
+; CHECK-NEXT: [[PHI:%.*]] = phi i64 [ [[TMP1]], %[[BB_1]] ], [ [[TMP1]], %[[BB_1]] ], [ 10, %[[ENTRY]] ], [ 10, %[[ENTRY]] ]
+; CHECK-NEXT: store i64 [[PHI]], ptr addrspace(1) [[OUT]], align 8
+; CHECK-NEXT: br label %[[BB_3]]
+; CHECK: [[BB_3]]:
+; CHECK-NEXT: ret void
+;
+entry:
+ switch i32 0, label %bb.3 [
+ i32 18, label %bb.2
+ i32 1, label %bb.2
+ i32 0, label %bb.3
+ ]
+bb.1:
+ switch i32 0, label %bb.3 [
+ i32 18, label %bb.2
+ i32 1, label %bb.2
+ i32 0, label %bb.3
+ ]
+
+bb.2:
+ %phi = phi i64 [ ptrtoint (ptr addrspacecast (ptr addrspace(3) @lds to ptr) to i64), %bb.1 ], [ ptrtoint (ptr addrspacecast (ptr addrspace(3) @lds to ptr) to i64), %bb.1 ], [10, %entry], [10, %entry]
+ store i64 %phi, ptr addrspace(1) %out, align 8
+ br label %bb.3
+
+bb.3:
+ ret void
+}
|
shiltian
commented
Nov 22, 2025
| // We need to cache the instructions we've already expanded to avoid expanding | ||
| // the same constant multiple times in the same basic block, which is | ||
| // problematic when the same constant is used in a phi node multiple times. | ||
| DenseMap<std::pair<Constant *, BasicBlock *>, SmallVector<Instruction *, 4>> |
Contributor
Author
There was a problem hiding this comment.
I suppose the expansion of the SmallVector will not cause any trouble in the map, since it is fixed size, and when it needs expansion, it uses heap and its original location will not be moved.
🐧 Linux x64 Test Results
Failed Tests(click on a test name to see its output) LLVMLLVM.CodeGen/AMDGPU/lower-kernel-lds-constexpr.llLLVM.CodeGen/AMDGPU/lower-module-lds-constantexpr.llIf these failures are unrelated to your changes (for example tests are broken or flaky at HEAD), please open an issue at https://github.com/llvm/llvm-project/issues and add the |
shiltian
commented
Nov 22, 2025
| ] | ||
|
|
||
| bb.2: | ||
| %phi = phi i64 [ ptrtoint (ptr addrspacecast (ptr addrspace(3) @lds to ptr) to i64), %bb.1 ], [ ptrtoint (ptr addrspacecast (ptr addrspace(3) @lds to ptr) to i64), %bb.1 ], [10, %entry], [10, %entry] |
Contributor
Author
There was a problem hiding this comment.
On a second thought, should we deduplicate the incoming BBs when we construct a phi node? In this way we don't need to fix anything else.
wenju-he
reviewed
Nov 22, 2025
| SmallVector<Instruction *, 4> &NewInsts = | ||
| ConstantToInstructionMap[std::make_pair(C, BI->getParent())]; | ||
| if (NewInsts.empty()) | ||
| NewInsts = expandUser(BI, C); |
Contributor
There was a problem hiding this comment.
when NewInsts.empty() is true, there will be a copy to NewInsts after it is default constructed. Can we avoid the copy?
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.
Fixes #167500.