[Inline] Introduce Constant::hasOneLiveUse, use it instead of hasOneUse in inline cost model (PR51667)

Otherwise, inlining costs may be pessimized by dead constants.

Fixes https://bugs.llvm.org/show_bug.cgi?id=51667.

Reviewed By: mtrofin, aeubanks

Differential Revision: https://reviews.llvm.org/D109294
This commit is contained in:
Erik Desjardins 2021-10-07 08:14:56 -07:00 committed by Mircea Trofin
parent 3a5aa57c9b
commit 11c8efd4db
5 changed files with 84 additions and 20 deletions

View File

@ -198,6 +198,12 @@ public:
/// hanging off of the globals.
void removeDeadConstantUsers() const;
/// Return true if the constant has exactly one live use.
///
/// This returns the same result as calling Value::hasOneUse after
/// Constant::removeDeadConstantUsers, but doesn't remove dead constants.
bool hasOneLiveUse() const;
const Constant *stripPointerCasts() const {
return cast<Constant>(Value::stripPointerCasts());
}

View File

@ -1859,8 +1859,8 @@ void InlineCostCallAnalyzer::updateThreshold(CallBase &Call, Function &Callee) {
SingleBBBonus = Threshold * SingleBBBonusPercent / 100;
VectorBonus = Threshold * VectorBonusPercent / 100;
bool OnlyOneCallAndLocalLinkage =
F.hasLocalLinkage() && F.hasOneUse() && &F == Call.getCalledFunction();
bool OnlyOneCallAndLocalLinkage = F.hasLocalLinkage() && F.hasOneLiveUse() &&
&F == Call.getCalledFunction();
// If there is only one call of the function, and it has internal linkage,
// the cost of inlining it drops dramatically. It may seem odd to update
// Cost in updateThreshold, but the bonus depends on the logic in this method.
@ -2665,7 +2665,7 @@ InlineResult CallAnalyzer::analyze() {
onBlockAnalyzed(BB);
}
bool OnlyOneCallAndLocalLinkage = F.hasLocalLinkage() && F.hasOneUse() &&
bool OnlyOneCallAndLocalLinkage = F.hasLocalLinkage() && F.hasOneLiveUse() &&
&F == CandidateCall.getCalledFunction();
// If this is a noduplicate call, we can still inline as long as
// inlining this would cause the removal of the caller (so the instruction

View File

@ -714,29 +714,41 @@ Constant::PossibleRelocationsTy Constant::getRelocationInfo() const {
return Result;
}
/// If the specified constantexpr is dead, remove it. This involves recursively
/// eliminating any dead users of the constantexpr.
static bool removeDeadUsersOfConstant(const Constant *C) {
/// Return true if the specified constantexpr is dead. This involves
/// recursively traversing users of the constantexpr.
/// If RemoveDeadUsers is true, also remove dead users at the same time.
static bool constantIsDead(const Constant *C, bool RemoveDeadUsers) {
if (isa<GlobalValue>(C)) return false; // Cannot remove this
while (!C->use_empty()) {
const Constant *User = dyn_cast<Constant>(C->user_back());
Value::const_user_iterator I = C->user_begin(), E = C->user_end();
while (I != E) {
const Constant *User = dyn_cast<Constant>(*I);
if (!User) return false; // Non-constant usage;
if (!removeDeadUsersOfConstant(User))
if (!constantIsDead(User, RemoveDeadUsers))
return false; // Constant wasn't dead
// Just removed User, so the iterator was invalidated.
// Since we return immediately upon finding a live user, we can always
// restart from user_begin().
if (RemoveDeadUsers)
I = C->user_begin();
else
++I;
}
// If C is only used by metadata, it should not be preserved but should have
// its uses replaced.
if (C->isUsedByMetadata()) {
const_cast<Constant *>(C)->replaceAllUsesWith(
UndefValue::get(C->getType()));
if (RemoveDeadUsers) {
// If C is only used by metadata, it should not be preserved but should
// have its uses replaced.
if (C->isUsedByMetadata()) {
const_cast<Constant *>(C)->replaceAllUsesWith(
UndefValue::get(C->getType()));
}
const_cast<Constant *>(C)->destroyConstant();
}
const_cast<Constant*>(C)->destroyConstant();
return true;
}
void Constant::removeDeadConstantUsers() const {
Value::const_user_iterator I = user_begin(), E = user_end();
Value::const_user_iterator LastNonDeadUser = E;
@ -748,7 +760,7 @@ void Constant::removeDeadConstantUsers() const {
continue;
}
if (!removeDeadUsersOfConstant(User)) {
if (!constantIsDead(User, /* RemoveDeadUsers= */ true)) {
// If the constant wasn't dead, remember that this was the last live use
// and move on to the next constant.
LastNonDeadUser = I;
@ -764,6 +776,20 @@ void Constant::removeDeadConstantUsers() const {
}
}
bool Constant::hasOneLiveUse() const {
unsigned NumUses = 0;
for (const Use &use : uses()) {
const Constant *User = dyn_cast<Constant>(use.getUser());
if (!User || !constantIsDead(User, /* RemoveDeadUsers= */ false)) {
++NumUses;
if (NumUses > 1)
return false;
}
}
return NumUses == 1;
}
Constant *Constant::replaceUndefsWith(Constant *C, Constant *Replacement) {
assert(C && Replacement && "Expected non-nullptr constant arguments");
Type *Ty = C->getType();

View File

@ -0,0 +1,32 @@
; Test to ensure that the inlining cost model isn't tripped up by dead constant users.
; In this case, the call to g via a bitcasted function pointer is canonicalized to
; a direct call to g with bitcasted arguments, leaving the original bitcast
; as a dead use of g.
; RUN: opt < %s -instcombine -inline -pass-remarks=inline -S 2>&1 \
; RUN: | FileCheck %s
; Inline costs of f and g should be the same.
; CHECK: 'f' inlined into 'h' with (cost=[[EXPECTED_COST:.+]], threshold={{.+}})
; CHECK: 'g' inlined into 'h' with (cost=[[EXPECTED_COST]], threshold={{.+}})
%0 = type { i64, i64, i64 }
%1 = type { i64, i64, i64 }
define internal void @f(%0* align 8 %a) unnamed_addr {
start:
ret void
}
define internal void @g(%0* align 8 %a) unnamed_addr {
start:
ret void
}
define void @h(%0* align 8 %a, %1* align 8 %b) unnamed_addr {
start:
call void @f(%0* align 8 %a)
call void bitcast (void (%0*)* @g to void (%1*)*)(%1* align 8 %b)
ret void
}

View File

@ -260,10 +260,10 @@ entry:
; constant expression cannot be inlined because the constant expression forms
; a second use. If this part starts failing we need to use more complex
; constant expressions to reference a particular function with them.
%sink = alloca i1
store volatile i1 icmp ne (i64 ptrtoint (void (i1)* @test4_g to i64), i64 ptrtoint(void (i1)* @test4_g to i64)), i1* %sink
%sink = alloca i64
store volatile i64 mul (i64 ptrtoint (void (i1)* @test4_g to i64), i64 ptrtoint(void (i1)* @test4_g to i64)), i64* %sink
call void @test4_g(i1 true)
; CHECK: store volatile i1 false
; CHECK: store volatile i64 mul (i64 ptrtoint (void (i1)* @test4_g to i64), i64 ptrtoint (void (i1)* @test4_g to i64)), i64* %sink
; CHECK: call void @test4_g(i1 true)
ret void