Skip to content

[PPCMergeStringPool] Avoid replacing constant with instruction #88846

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
May 9, 2024
Merged

[PPCMergeStringPool] Avoid replacing constant with instruction #88846

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ef11bcc
[PPCMergeStringPool] Avoid replacing constant with instruction
nikic Apr 16, 2024
bbf1144
drop assertion
nikic May 1, 2024
d7b746e
Also check for EHPads
nikic May 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 18 additions & 39 deletions llvm/lib/Target/PowerPC/PPCMergeStringPool.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -117,9 +118,20 @@ 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 (auto *I = dyn_cast<Instruction>(CurrentUser)) {
// Do not merge globals in exception pads.
if (I->isEHPad())
return false;

if (auto *II = dyn_cast<IntrinsicInst>(I)) {
// Some intrinsics require a plain global.
if (II->getIntrinsicID() == Intrinsic::eh_typeid_for)
return false;
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From

llvm-project/llvm/lib/CodeGen/GlobalMerge.cpp

Lines 601 to 610 in 853344d

// Keep globals used by landingpads and catchpads.
for (const Use &U : Pad->operands()) {
if (const GlobalVariable *GV =
dyn_cast<GlobalVariable>(U->stripPointerCasts()))
MustKeepGlobalVariables.insert(GV);
else if (const ConstantArray *CA = dyn_cast<ConstantArray>(U->stripPointerCasts())) {
for (const Use &Elt : CA->operands()) {
if (const GlobalVariable *GV =
dyn_cast<GlobalVariable>(Elt->stripPointerCasts()))
MustKeepGlobalVariables.insert(GV);
, seems the 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went ahead and added the EHPad check while also keeping the explicit eh.typeid.for check. Better safe than sorry...

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough.

// 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
Expand Down Expand Up @@ -314,14 +326,6 @@ void PPCMergeStringPool::replaceUsesWithGEP(GlobalVariable *GlobalToReplace,
Users.push_back(CurrentUser);

for (User *CurrentUser : Users) {
Instruction *UserInstruction = dyn_cast<Instruction>(CurrentUser);
Constant *UserConstant = dyn_cast<Constant>(CurrentUser);

// At this point we expect that the user is either an instruction or a
// constant.
assert((UserConstant || UserInstruction) &&
"Expected the user to be an instruction or a constant.");

// The user was not found so it must have been replaced earlier.
if (!userHasOperand(CurrentUser, GlobalToReplace))
continue;
Expand All @@ -330,38 +334,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(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks reasonable to me.

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);
}
}

Expand Down
6 changes: 2 additions & 4 deletions llvm/test/CodeGen/PowerPC/merge-string-used-by-metadata.mir
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
47 changes: 47 additions & 0 deletions llvm/test/CodeGen/PowerPC/mergeable-string-pool-exceptions.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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, .Ldummy@toc@ha
; CHECK-NEXT: addi 3, 3, .Ldummy@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()
18 changes: 8 additions & 10 deletions llvm/test/CodeGen/PowerPC/mergeable-string-pool-pass-only.mir
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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
}

Expand All @@ -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
}

Expand All @@ -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
}

Expand All @@ -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
Expand Down
Loading