[PPCMergeStringPool] Avoid replacing constant with instruction #88846
Conversation
This file contains 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
|
@llvm/pr-subscribers-backend-powerpc Author: Nikita Popov (nikic) ChangesString pool merging currently, for a reason that's not entirely clear to me, tries to create GEP instructions instead of GEP constant expressions when replacing constant references. It only uses constant expressions in cases where this is required. However, it does not catch all cases where such a requirement exists. For example, the landingpad catch clause has to be a constant. Fix this by always using the constant expression variant, which also makes the implementation simpler. Additionally, there are some edge cases where even replacement with a constant GEP is not legal. The one I am aware of is the llvm.eh.typeid.for intrinsic, so add a special case to forbid replacements for it. Fixes #88844. Full diff: https://github.com/llvm/llvm-project/pull/88846.diff 4 Files Affected:
diff --git a/llvm/lib/Target/PowerPC/PPCMergeStringPool.cpp b/llvm/lib/Target/PowerPC/PPCMergeStringPool.cpp
index 76d60c28f1e445..60db2772517b20 100644
--- a/llvm/lib/Target/PowerPC/PPCMergeStringPool.cpp
+++ b/llvm/lib/Target/PowerPC/PPCMergeStringPool.cpp
@@ -23,6 +23,7 @@
#include "llvm/Analysis/ScalarEvolutionAliasAnalysis.h"
#include "llvm/IR/Constants.h"
#include "llvm/IR/Instructions.h"
+#include "llvm/IR/IntrinsicInst.h"
#include "llvm/IR/Module.h"
#include "llvm/IR/ValueSymbolTable.h"
#include "llvm/Pass.h"
@@ -117,9 +118,16 @@ class PPCMergeStringPool : public ModulePass {
// sure that they can be replaced.
static bool hasReplaceableUsers(GlobalVariable &GV) {
for (User *CurrentUser : GV.users()) {
- // Instruction users are always valid.
- if (isa<Instruction>(CurrentUser))
+ if (isa<Instruction>(CurrentUser)) {
+ if (auto *II = dyn_cast<IntrinsicInst>(CurrentUser)) {
+ // Some intrinsics require a plain global.
+ if (II->getIntrinsicID() == Intrinsic::eh_typeid_for)
+ return false;
+ }
+
+ // Other instruction users are always valid.
continue;
+ }
// We cannot replace GlobalValue users because they are not just nodes
// in IR. To replace a user like this we would need to create a new
@@ -330,38 +338,13 @@ void PPCMergeStringPool::replaceUsesWithGEP(GlobalVariable *GlobalToReplace,
if (isa<GlobalValue>(CurrentUser))
continue;
- if (!UserInstruction) {
- // User is a constant type.
- Constant *ConstGEP = ConstantExpr::getInBoundsGetElementPtr(
- PooledStructType, GPool, Indices);
- UserConstant->handleOperandChange(GlobalToReplace, ConstGEP);
- continue;
- }
-
- if (PHINode *UserPHI = dyn_cast<PHINode>(UserInstruction)) {
- // GEP instructions cannot be added before PHI nodes.
- // With getInBoundsGetElementPtr we create the GEP and then replace it
- // inline into the PHI.
- Constant *ConstGEP = ConstantExpr::getInBoundsGetElementPtr(
- PooledStructType, GPool, Indices);
- UserPHI->replaceUsesOfWith(GlobalToReplace, ConstGEP);
- continue;
- }
- // The user is a valid instruction that is not a PHINode.
- GetElementPtrInst *GEPInst =
- GetElementPtrInst::Create(PooledStructType, GPool, Indices);
- GEPInst->insertBefore(UserInstruction);
-
- LLVM_DEBUG(dbgs() << "Inserting GEP before:\n");
- LLVM_DEBUG(UserInstruction->dump());
-
+ Constant *ConstGEP = ConstantExpr::getInBoundsGetElementPtr(
+ PooledStructType, GPool, Indices);
LLVM_DEBUG(dbgs() << "Replacing this global:\n");
LLVM_DEBUG(GlobalToReplace->dump());
LLVM_DEBUG(dbgs() << "with this:\n");
- LLVM_DEBUG(GEPInst->dump());
-
- // After the GEP is inserted the GV can be replaced.
- CurrentUser->replaceUsesOfWith(GlobalToReplace, GEPInst);
+ LLVM_DEBUG(ConstGEP->dump());
+ GlobalToReplace->replaceAllUsesWith(ConstGEP);
}
}
diff --git a/llvm/test/CodeGen/PowerPC/merge-string-used-by-metadata.mir b/llvm/test/CodeGen/PowerPC/merge-string-used-by-metadata.mir
index 2a791966be4ecf..4a40974a2a227b 100644
--- a/llvm/test/CodeGen/PowerPC/merge-string-used-by-metadata.mir
+++ b/llvm/test/CodeGen/PowerPC/merge-string-used-by-metadata.mir
@@ -14,16 +14,14 @@
define noundef ptr @func1(ptr noundef nonnull align 8 dereferenceable(8) %this) #0 !dbg !6 {
; CHECK-LABEL: func1
- ; CHECK: %0 = getelementptr { [7 x i8], [7 x i8] }, ptr @__ModuleStringPool, i32 0, i32 1
- ; CHECK-NEXT: ret ptr %0, !dbg !14
+ ; CHECK: ret ptr getelementptr inbounds ({ [7 x i8], [7 x i8] }, ptr @__ModuleStringPool, i32 0, i32 1), !dbg !14
entry:
ret ptr @const.2, !dbg !14
}
define noundef ptr @func2(ptr noundef nonnull align 8 dereferenceable(8) %this) #0 {
; CHECK-LABEL: func2
- ; CHECK: %0 = getelementptr { [7 x i8], [7 x i8] }, ptr @__ModuleStringPool, i32 0, i32 0
- ; CHECK-NEXT: ret ptr %0
+ ; CHECK: ret ptr @__ModuleStringPool
entry:
ret ptr @const.1
}
diff --git a/llvm/test/CodeGen/PowerPC/mergeable-string-pool-exceptions.ll b/llvm/test/CodeGen/PowerPC/mergeable-string-pool-exceptions.ll
new file mode 100644
index 00000000000000..0489c74c0f819f
--- /dev/null
+++ b/llvm/test/CodeGen/PowerPC/mergeable-string-pool-exceptions.ll
@@ -0,0 +1,47 @@
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py UTC_ARGS: --version 4
+; RUN: llc -mtriple=ppc64le-unknown-linux-gnu < %s | FileCheck %s
+
+@id = private unnamed_addr constant [4 x i8] c"@id\00", align 1
+@id2 = private unnamed_addr constant [5 x i8] c"@id2\00", align 1
+
+; Higher-aligned dummy to make sure it is first in the string pool.
+@dummy = private unnamed_addr constant [1 x i32] [i32 42], align 4
+
+define ptr @test1() personality ptr @__gnu_objc_personality_v0 {
+; CHECK-LABEL: test1:
+; CHECK: # %bb.0:
+; CHECK-NEXT: mflr 0
+; CHECK-NEXT: stdu 1, -32(1)
+; CHECK-NEXT: std 0, 48(1)
+; CHECK-NEXT: .cfi_def_cfa_offset 32
+; CHECK-NEXT: .cfi_offset lr, 16
+; CHECK-NEXT: addis 3, 2, .L__ModuleStringPool@toc@ha
+; CHECK-NEXT: addi 3, 3, .L__ModuleStringPool@toc@l
+; CHECK-NEXT: bl foo
+; CHECK-NEXT: nop
+ invoke void @foo(ptr @dummy)
+ to label %cont unwind label %unwind
+
+cont:
+ unreachable
+
+unwind:
+ %lp = landingpad { ptr, i32 }
+ catch ptr @id
+ resume { ptr, i32 } %lp
+}
+
+define i32 @test2() personality ptr @__gnu_objc_personality_v0 {
+; CHECK-LABEL: test2:
+; CHECK: # %bb.0:
+; CHECK-NEXT: li 3, 1
+; CHECK-NEXT: blr
+ %id = tail call i32 @llvm.eh.typeid.for(ptr @id2)
+ ret i32 %id
+}
+
+declare i32 @__gnu_objc_personality_v0(...)
+
+declare i32 @llvm.eh.typeid.for(ptr)
+
+declare void @foo()
diff --git a/llvm/test/CodeGen/PowerPC/mergeable-string-pool-pass-only.mir b/llvm/test/CodeGen/PowerPC/mergeable-string-pool-pass-only.mir
index e2fb0ced8f3490..3d8afb604fd3ca 100644
--- a/llvm/test/CodeGen/PowerPC/mergeable-string-pool-pass-only.mir
+++ b/llvm/test/CodeGen/PowerPC/mergeable-string-pool-pass-only.mir
@@ -35,8 +35,7 @@
ret i32 %call
; CHECK-LABEL: test1
- ; CHECK: %0 = getelementptr { [7 x double], [7 x double], [6 x i32], [6 x i32], [7 x float], [7 x float], [8 x i8], [16 x i8] }, ptr @__ModuleStringPool, i32 0, i32 6
- ; CHECK: tail call signext i32 @calleeStr
+ ; CHECK: %call = tail call signext i32 @calleeStr(ptr noundef nonnull getelementptr inbounds ({ [7 x double], [7 x double], [6 x i32], [6 x i32], [7 x float], [7 x float], [8 x i8], [16 x i8] }, ptr @__ModuleStringPool, i32 0, i32 6))
}
define dso_local signext i32 @test2() local_unnamed_addr #0 {
@@ -49,7 +48,7 @@
ret i32 %call
; CHECK-LABEL: test2
- ; CHECK: %0 = getelementptr { [7 x double], [7 x double], [6 x i32], [6 x i32], [7 x float], [7 x float], [8 x i8], [16 x i8] }, ptr @__ModuleStringPool, i32 0, i32 2
+ ; CHECK: call void @llvm.memcpy.p0.p0.i64(ptr noundef nonnull align 4 dereferenceable(24) %A, ptr noundef nonnull align 4 dereferenceable(24) getelementptr inbounds ({ [7 x double], [7 x double], [6 x i32], [6 x i32], [7 x float], [7 x float], [8 x i8], [16 x i8] }, ptr @__ModuleStringPool, i32 0, i32 2), i64 24, i1 false)
; CHECK: call signext i32 @calleeInt
}
@@ -62,7 +61,7 @@
call void @llvm.lifetime.end.p0(i64 28, ptr nonnull %A) #0
ret i32 %call
; CHECK-LABEL: test3
- ; CHECK: %0 = getelementptr { [7 x double], [7 x double], [6 x i32], [6 x i32], [7 x float], [7 x float], [8 x i8], [16 x i8] }, ptr @__ModuleStringPool, i32 0, i32 4
+ ; CHECK: call void @llvm.memcpy.p0.p0.i64(ptr noundef nonnull align 4 dereferenceable(28) %A, ptr noundef nonnull align 4 dereferenceable(28) getelementptr inbounds ({ [7 x double], [7 x double], [6 x i32], [6 x i32], [7 x float], [7 x float], [8 x i8], [16 x i8] }, ptr @__ModuleStringPool, i32 0, i32 4), i64 28, i1 false)
; CHECK: call signext i32 @calleeFloat
}
@@ -75,7 +74,7 @@
call void @llvm.lifetime.end.p0(i64 56, ptr nonnull %A) #0
ret i32 %call
; CHECK-LABEL: test4
- ; CHECK: %0 = getelementptr { [7 x double], [7 x double], [6 x i32], [6 x i32], [7 x float], [7 x float], [8 x i8], [16 x i8] }, ptr @__ModuleStringPool, i32 0, i32 0
+ ; CHECK: call void @llvm.memcpy.p0.p0.i64(ptr noundef nonnull align 8 dereferenceable(56) %A, ptr noundef nonnull align 8 dereferenceable(56) @__ModuleStringPool, i64 56, i1 false)
; CHECK: call signext i32 @calleeDouble
}
@@ -102,11 +101,10 @@
call void @llvm.lifetime.end.p0(i64 24, ptr nonnull %B) #0
ret i32 %add7
; CHECK-LABEL: test5
- ; CHECK: %0 = getelementptr { [7 x double], [7 x double], [6 x i32], [6 x i32], [7 x float], [7 x float], [8 x i8], [16 x i8] }, ptr @__ModuleStringPool, i32 0, i32 3
- ; CHECK: %1 = getelementptr { [7 x double], [7 x double], [6 x i32], [6 x i32], [7 x float], [7 x float], [8 x i8], [16 x i8] }, ptr @__ModuleStringPool, i32 0, i32 5
- ; CHECK: %2 = getelementptr { [7 x double], [7 x double], [6 x i32], [6 x i32], [7 x float], [7 x float], [8 x i8], [16 x i8] }, ptr @__ModuleStringPool, i32 0, i32 1
- ; CHECK: %3 = getelementptr { [7 x double], [7 x double], [6 x i32], [6 x i32], [7 x float], [7 x float], [8 x i8], [16 x i8] }, ptr @__ModuleStringPool, i32 0, i32 7
- ; CHECK: call signext i32 @calleeStr
+ ; CHECK: call void @llvm.memcpy.p0.p0.i64(ptr noundef nonnull align 4 dereferenceable(24) %B, ptr noundef nonnull align 4 dereferenceable(24) getelementptr inbounds ({ [7 x double], [7 x double], [6 x i32], [6 x i32], [7 x float], [7 x float], [8 x i8], [16 x i8] }, ptr @__ModuleStringPool, i32 0, i32 3), i64 24, i1 false)
+ ; CHECK: call void @llvm.memcpy.p0.p0.i64(ptr noundef nonnull align 4 dereferenceable(28) %C, ptr noundef nonnull align 4 dereferenceable(28) getelementptr inbounds ({ [7 x double], [7 x double], [6 x i32], [6 x i32], [7 x float], [7 x float], [8 x i8], [16 x i8] }, ptr @__ModuleStringPool, i32 0, i32 5), i64 28, i1 false)
+ ; CHECK: call void @llvm.memcpy.p0.p0.i64(ptr noundef nonnull align 8 dereferenceable(56) %D, ptr noundef nonnull align 8 dereferenceable(56) getelementptr inbounds ({ [7 x double], [7 x double], [6 x i32], [6 x i32], [7 x float], [7 x float], [8 x i8], [16 x i8] }, ptr @__ModuleStringPool, i32 0, i32 1), i64 56, i1 false)
+ ; CHECK: call signext i32 @calleeStr(ptr noundef nonnull getelementptr inbounds ({ [7 x double], [7 x double], [6 x i32], [6 x i32], [7 x float], [7 x float], [8 x i8], [16 x i8] }, ptr @__ModuleStringPool, i32 0, i32 7))
; CHECK: call signext i32 @calleeInt
; CHECK: call signext i32 @calleeFloat
; CHECK: call signext i32 @calleeDouble
|
|
ping :) |
| if (II->getIntrinsicID() == Intrinsic::eh_typeid_for) | ||
| return false; | ||
| } | ||
|
|
There was a problem hiding this comment.
From
llvm-project/llvm/lib/CodeGen/GlobalMerge.cpp
Lines 601 to 610 in 853344d
GlobalVariable used in eh related instructions are not allowed to replace. Not sure why we don't have similar logic here. @stefanp-ibm do you know why? Thanks.
There was a problem hiding this comment.
Hm, maybe adding this check is a better fix. I assume it will also cover the eh.typeid.for issue if this intrinsic is only used if the global is also mentioned in a landingpad.
There was a problem hiding this comment.
I went ahead and added the EHPad check while also keeping the explicit eh.typeid.for check. Better safe than sorry...
| LLVM_DEBUG(dbgs() << "Inserting GEP before:\n"); | ||
| LLVM_DEBUG(UserInstruction->dump()); | ||
|
|
||
| Constant *ConstGEP = ConstantExpr::getInBoundsGetElementPtr( |
There was a problem hiding this comment.
Looks reasonable to me.
String pool merging currently, for a reason that's not entirely clear to me, tries to create GEP instructions instead of GEP constant expressions when replacing constant references. It only uses constant expressions in cases where this is required. However, it does not catch all cases where such a requirement exists. For example, the landingpad catch clause has to be a constant. Fix this by always using the constant expression variant, which also makes the implementation simpler. Additionally, there are some edge cases where even replacement with a constant GEP is not legal. The one I am aware of is the llvm.eh.typeid.for intrinsic, so add a special case to forbid replacements for it. Fixes llvm#88844.
chenzheng1030
approved these changes
May 9, 2024
| if (II->getIntrinsicID() == Intrinsic::eh_typeid_for) | ||
| return false; | ||
| } | ||
|
|
nikic
added a commit
to nikic/llvm-project
that referenced
this pull request
May 9, 2024
…88846) String pool merging currently, for a reason that's not entirely clear to me, tries to create GEP instructions instead of GEP constant expressions when replacing constant references. It only uses constant expressions in cases where this is required. However, it does not catch all cases where such a requirement exists. For example, the landingpad catch clause has to be a constant. Fix this by always using the constant expression variant, which also makes the implementation simpler. Additionally, there are some edge cases where even replacement with a constant GEP is not legal. The one I am aware of is the llvm.eh.typeid.for intrinsic, so add a special case to forbid replacements for it. Fixes llvm#88844. (cherry picked from commit 3a3aeb8)
nikic
added a commit
to nikic/llvm-project
that referenced
this pull request
May 9, 2024
…88846) String pool merging currently, for a reason that's not entirely clear to me, tries to create GEP instructions instead of GEP constant expressions when replacing constant references. It only uses constant expressions in cases where this is required. However, it does not catch all cases where such a requirement exists. For example, the landingpad catch clause has to be a constant. Fix this by always using the constant expression variant, which also makes the implementation simpler. Additionally, there are some edge cases where even replacement with a constant GEP is not legal. The one I am aware of is the llvm.eh.typeid.for intrinsic, so add a special case to forbid replacements for it. Fixes llvm#88844. (cherry picked from commit 3a3aeb8)
tstellar
pushed a commit
to nikic/llvm-project
that referenced
this pull request
May 14, 2024
…88846) String pool merging currently, for a reason that's not entirely clear to me, tries to create GEP instructions instead of GEP constant expressions when replacing constant references. It only uses constant expressions in cases where this is required. However, it does not catch all cases where such a requirement exists. For example, the landingpad catch clause has to be a constant. Fix this by always using the constant expression variant, which also makes the implementation simpler. Additionally, there are some edge cases where even replacement with a constant GEP is not legal. The one I am aware of is the llvm.eh.typeid.for intrinsic, so add a special case to forbid replacements for it. Fixes llvm#88844. (cherry picked from commit 3a3aeb8)
nikic
added a commit
to nikic/llvm-project
that referenced
this pull request
May 22, 2024
In llvm#88846 I changed this code to use RAUW to perform the replacement instead of manual updates -- but kept the outer loop, which means we try to perform RAUW once per user. However, some of the users might be freed by the RAUW operation, resulting in use-after-free. I think the case where this happens is constant users where the replacement might result in the destruction of the original constant. I wasn't able to come up with a test case though. This is intended to fix llvm#92991.
nikic
added a commit
to nikic/llvm-project
that referenced
this pull request
May 23, 2024
In llvm#88846 I changed this code to use RAUW to perform the replacement instead of manual updates -- but kept the outer loop, which means we try to perform RAUW once per user. However, some of the users might be freed by the RAUW operation, resulting in use-after-free. I think the case where this happens is constant users where the replacement might result in the destruction of the original constant. I wasn't able to come up with a test case though. This is intended to fix llvm#92991.
nikic
added a commit
that referenced
this pull request
May 27, 2024
In #88846 I changed this code to use RAUW to perform the replacement instead of manual updates -- but kept the outer loop, which means we try to perform RAUW once per user. However, some of the users might be freed by the RAUW operation, resulting in use-after-free. The case where this happens is constant users where the replacement might result in the destruction of the original constant. Fixes #92991.
llvmbot
pushed a commit
to llvmbot/llvm-project
that referenced
this pull request
May 27, 2024
In llvm#88846 I changed this code to use RAUW to perform the replacement instead of manual updates -- but kept the outer loop, which means we try to perform RAUW once per user. However, some of the users might be freed by the RAUW operation, resulting in use-after-free. The case where this happens is constant users where the replacement might result in the destruction of the original constant. Fixes llvm#92991. (cherry picked from commit 9f85bc8)
llvmbot
pushed a commit
to llvmbot/llvm-project
that referenced
this pull request
Jun 4, 2024
In llvm#88846 I changed this code to use RAUW to perform the replacement instead of manual updates -- but kept the outer loop, which means we try to perform RAUW once per user. However, some of the users might be freed by the RAUW operation, resulting in use-after-free. The case where this happens is constant users where the replacement might result in the destruction of the original constant. Fixes llvm#92991. (cherry picked from commit 9f85bc8)
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.
String pool merging currently, for a reason that's not entirely clear to me, tries to create GEP instructions instead of GEP constant expressions when replacing constant references. It only uses constant expressions in cases where this is required. However, it does not catch all cases where such a requirement exists. For example, the landingpad catch clause has to be a constant.
Fix this by always using the constant expression variant, which also makes the implementation simpler.
Additionally, there are some edge cases where even replacement with a constant GEP is not legal. The one I am aware of is the llvm.eh.typeid.for intrinsic, so add a special case to forbid replacements for it.
Fixes #88844.