Skip to content

[FuncSpec] Only compute Latency bonus when necessary #113159

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
hazzlim merged 4 commits into from
Oct 23, 2024
Merged

[FuncSpec] Only compute Latency bonus when necessary #113159

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d382315
[FuncSpec] Only compute Latency bonus when necessary
hazzlim Oct 16, 2024
fc3aaa4
Address review comments
hazzlim Oct 21, 2024
b9336f2
Address review comments 2
hazzlim Oct 21, 2024
0a5d644
Address review 3
hazzlim Oct 22, 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
23 changes: 13 additions & 10 deletions llvm/include/llvm/Transforms/IPO/FunctionSpecialization.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,8 +173,9 @@ struct Bonus {
};

class InstCostVisitor : public InstVisitor<InstCostVisitor, Constant *> {
std::function<BlockFrequencyInfo &(Function &)> GetBFI;
Function *F;
const DataLayout &DL;
BlockFrequencyInfo &BFI;
TargetTransformInfo &TTI;
SCCPSolver &Solver;

Expand All @@ -192,26 +193,29 @@ class InstCostVisitor : public InstVisitor<InstCostVisitor, Constant *> {
ConstMap::iterator LastVisited;

public:
InstCostVisitor(const DataLayout &DL, BlockFrequencyInfo &BFI,
TargetTransformInfo &TTI, SCCPSolver &Solver)
: DL(DL), BFI(BFI), TTI(TTI), Solver(Solver) {}
InstCostVisitor(std::function<BlockFrequencyInfo &(Function &)> GetBFI,
Function *F, const DataLayout &DL, TargetTransformInfo &TTI,
SCCPSolver &Solver)
: GetBFI(GetBFI), F(F), DL(DL), TTI(TTI), Solver(Solver) {}

bool isBlockExecutable(BasicBlock *BB) {
return Solver.isBlockExecutable(BB) && !DeadBlocks.contains(BB);
}

Bonus getSpecializationBonus(Argument *A, Constant *C);
Cost getCodeSizeBonus(Argument *A, Constant *C);

Cost getCodeSizeBonusFromPendingPHIs();

Bonus getBonusFromPendingPHIs();
Cost getLatencyBonus();

private:
friend class InstVisitor<InstCostVisitor, Constant *>;

static bool canEliminateSuccessor(BasicBlock *BB, BasicBlock *Succ,
DenseSet<BasicBlock *> &DeadBlocks);

Bonus getUserBonus(Instruction *User, Value *Use = nullptr,
Constant *C = nullptr);
Cost getUserCodeSizeBonus(Instruction *User, Value *Use = nullptr,
Constant *C = nullptr);

Cost estimateBasicBlocks(SmallVectorImpl<BasicBlock *> &WorkList);
Cost estimateSwitchInst(SwitchInst &I);
Expand Down Expand Up @@ -283,9 +287,8 @@ class FunctionSpecializer {
bool run();

InstCostVisitor getInstCostVisitorFor(Function *F) {
auto &BFI = GetBFI(*F);
auto &TTI = GetTTI(*F);
return InstCostVisitor(M.getDataLayout(), BFI, TTI, Solver);
return InstCostVisitor(GetBFI, F, M.getDataLayout(), TTI, Solver);
}

private:
Expand Down
85 changes: 51 additions & 34 deletions llvm/lib/Transforms/IPO/FunctionSpecialization.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ bool InstCostVisitor::canEliminateSuccessor(BasicBlock *BB, BasicBlock *Succ,
Cost InstCostVisitor::estimateBasicBlocks(
SmallVectorImpl<BasicBlock *> &WorkList) {
Cost CodeSize = 0;
// Accumulate the instruction cost of each basic block weighted by frequency.
// Accumulate the codesize savings of each basic block.
while (!WorkList.empty()) {
BasicBlock *BB = WorkList.pop_back_val();

Expand Down Expand Up @@ -154,37 +154,55 @@ static Constant *findConstantFor(Value *V, ConstMap &KnownConstants) {
return KnownConstants.lookup(V);
}

Bonus InstCostVisitor::getBonusFromPendingPHIs() {
Bonus B;
Cost InstCostVisitor::getCodeSizeBonusFromPendingPHIs() {
Cost CodeSize;
while (!PendingPHIs.empty()) {
Instruction *Phi = PendingPHIs.pop_back_val();
// The pending PHIs could have been proven dead by now.
if (isBlockExecutable(Phi->getParent()))
B += getUserBonus(Phi);
CodeSize += getUserCodeSizeBonus(Phi);
}
return B;
return CodeSize;
}

/// Compute a bonus for replacing argument \p A with constant \p C.
Bonus InstCostVisitor::getSpecializationBonus(Argument *A, Constant *C) {
/// Compute the codesize savings for replacing argument \p A with constant \p C.
Cost InstCostVisitor::getCodeSizeBonus(Argument *A, Constant *C) {
LLVM_DEBUG(dbgs() << "FnSpecialization: Analysing bonus for constant: "
<< C->getNameOrAsOperand() << "\n");
Bonus B;
Cost CodeSize;
for (auto *U : A->users())
if (auto *UI = dyn_cast<Instruction>(U))
if (isBlockExecutable(UI->getParent()))
B += getUserBonus(UI, A, C);
CodeSize += getUserCodeSizeBonus(UI, A, C);

LLVM_DEBUG(dbgs() << "FnSpecialization: Accumulated bonus {CodeSize = "
<< B.CodeSize << ", Latency = " << B.Latency
<< "} for argument " << *A << "\n");
return B;
<< CodeSize << "} for argument " << *A << "\n");
return CodeSize;
}

Cost InstCostVisitor::getLatencyBonus() {
auto &BFI = GetBFI(*F);
Cost Latency = 0;

for (auto Pair : KnownConstants) {
Instruction *I = dyn_cast<Instruction>(Pair.first);
if (!I)
continue;

uint64_t Weight = BFI.getBlockFreq(I->getParent()).getFrequency() /
BFI.getEntryFreq().getFrequency();
Latency +=
Copy link
Collaborator

@labrinea labrinea Oct 22, 2024

Choose a reason for hiding this comment

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

Sorry I missed it before. It seems worthwhile to add a debug message here showing how much is the latency saving and which instruction it corresponds to.

Copy link
Contributor Author

@hazzlim hazzlim Oct 22, 2024

Choose a reason for hiding this comment

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

Ah yep good point - I've added back the debug statements for each Instruction's Latency here.

Weight * TTI.getInstructionCost(I, TargetTransformInfo::TCK_Latency);
}

return Latency;
}

Bonus InstCostVisitor::getUserBonus(Instruction *User, Value *Use, Constant *C) {
Cost InstCostVisitor::getUserCodeSizeBonus(Instruction *User, Value *Use,
Constant *C) {
// We have already propagated a constant for this user.
if (KnownConstants.contains(User))
return {0, 0};
return 0;

// Cache the iterator before visiting.
LastVisited = Use ? KnownConstants.insert({Use, C}).first
Expand All @@ -198,7 +216,7 @@ Bonus InstCostVisitor::getUserBonus(Instruction *User, Value *Use, Constant *C)
} else {
C = visit(*User);
if (!C)
return {0, 0};
return 0;
}

// Even though it doesn't make sense to bind switch and branch instructions
Expand All @@ -208,23 +226,15 @@ Bonus InstCostVisitor::getUserBonus(Instruction *User, Value *Use, Constant *C)

CodeSize += TTI.getInstructionCost(User, TargetTransformInfo::TCK_CodeSize);

uint64_t Weight = BFI.getBlockFreq(User->getParent()).getFrequency() /
BFI.getEntryFreq().getFrequency();

Cost Latency = Weight *
TTI.getInstructionCost(User, TargetTransformInfo::TCK_Latency);

LLVM_DEBUG(dbgs() << "FnSpecialization: {CodeSize = " << CodeSize
<< ", Latency = " << Latency << "} for user "
<< *User << "\n");
<< "} for user " << *User << "\n");

Bonus B(CodeSize, Latency);
for (auto *U : User->users())
if (auto *UI = dyn_cast<Instruction>(U))
if (UI != User && isBlockExecutable(UI->getParent()))
B += getUserBonus(UI, User, C);
CodeSize += getUserCodeSizeBonus(UI, User, C);

return B;
return CodeSize;
}

Cost InstCostVisitor::estimateSwitchInst(SwitchInst &I) {
Expand Down Expand Up @@ -875,24 +885,23 @@ bool FunctionSpecializer::findSpecializations(Function *F, unsigned FuncSize,
AllSpecs[Index].CallSites.push_back(&CS);
} else {
// Calculate the specialisation gain.
Bonus B;
Cost CodeSize;
unsigned Score = 0;
InstCostVisitor Visitor = getInstCostVisitorFor(F);
for (ArgInfo &A : S.Args) {
B += Visitor.getSpecializationBonus(A.Formal, A.Actual);
CodeSize += Visitor.getCodeSizeBonus(A.Formal, A.Actual);
Score += getInliningBonus(A.Formal, A.Actual);
}
B += Visitor.getBonusFromPendingPHIs();

CodeSize += Visitor.getCodeSizeBonusFromPendingPHIs();

LLVM_DEBUG(dbgs() << "FnSpecialization: Specialization bonus {CodeSize = "
<< B.CodeSize << ", Latency = " << B.Latency
<< ", Inlining = " << Score << "}\n");
<< CodeSize << ", Inlining = " << Score << "}\n");

Bonus B = {CodeSize, 0};
Copy link
Collaborator

@labrinea labrinea Oct 21, 2024

Choose a reason for hiding this comment

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

The Bonus struct seems redundant now. We should probably remove its refinition from the header file. I would replace B with Cost CodeSizeSavings.

Copy link
Contributor Author

@hazzlim hazzlim Oct 21, 2024

Choose a reason for hiding this comment

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

Fair point - I was keeping it as it seemed like a reasonable way to abstract away the getValue() logic and to pass to the lambda, but I see it's probably cleaner without. Done.

FunctionGrowth[F] += FuncSize - B.CodeSize;

auto IsProfitable = [](Bonus &B, unsigned Score, unsigned FuncSize,
unsigned FuncGrowth) -> bool {
unsigned FuncGrowth, InstCostVisitor &V) -> bool {
// No check required.
if (ForceSpecialization)
return true;
Expand All @@ -902,6 +911,14 @@ bool FunctionSpecializer::findSpecializations(Function *F, unsigned FuncSize,
// Minimum codesize savings.
if (B.CodeSize < MinCodeSizeSavings * FuncSize / 100)
return false;

// Lazily compute the Latency, to avoid unnecessarily computing BFI.
B += {0, V.getLatencyBonus()};

LLVM_DEBUG(
dbgs() << "FnSpecialization: Specialization bonus {Latency = "
<< B.Latency << "}\n");

// Minimum latency savings.
if (B.Latency < MinLatencySavings * FuncSize / 100)
return false;
Expand All @@ -912,7 +929,7 @@ bool FunctionSpecializer::findSpecializations(Function *F, unsigned FuncSize,
};

// Discard unprofitable specialisations.
if (!IsProfitable(B, Score, FuncSize, FunctionGrowth[F]))
if (!IsProfitable(B, Score, FuncSize, FunctionGrowth[F], Visitor))
continue;

// Create a new specialisation entry.
Expand Down
18 changes: 9 additions & 9 deletions llvm/test/Transforms/SCCP/ipsccp-preserve-pdt.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,25 +4,25 @@

; This test case is trying to validate that the postdomtree is preserved
; correctly by the ipsccp pass. A tricky bug was introduced in commit
; 1b1232047e83b69561 when PDT would be feched using getCachedAnalysis in order
; 1b1232047e83b69561 when PDT would be fetched using getCachedAnalysis in order
; to setup a DomTreeUpdater (to update the PDT during transformation in order
; to preserve the analysis). But given that commit the PDT could end up being
; required and calculated via BlockFrequency analysis. So the problem was that
; when setting up the DomTreeUpdater we used a nullptr in case PDT wasn't
; cached at the begininng of IPSCCP, to indicate that no updates where needed
; cached at the beginning of IPSCCP, to indicate that no updates were needed
; for PDT. But then the PDT was calculated, given the input IR, and preserved
; using the non-updated state (as the DTU wasn't configured for updating the
; PDT).

; CHECK-NOT: <badref>
; CHECK: Inorder PostDominator Tree: DFSNumbers invalid: 0 slow queries.
; CHECK-NEXT: [1] <<exit node>> {4294967295,4294967295} [0]
; CHECK-NEXT: [2] %for.cond34 {4294967295,4294967295} [1]
; CHECK-NEXT: [3] %for.cond16 {4294967295,4294967295} [2]
; CHECK-NEXT: [2] %for.body {4294967295,4294967295} [1]
; CHECK-NEXT: [2] %if.end4 {4294967295,4294967295} [1]
; CHECK-NEXT: [3] %entry {4294967295,4294967295} [2]
; CHECK-NEXT: Roots: %for.cond34 %for.body
; CHECK-NEXT: [1] <<exit node>> {4294967295,4294967295} [0]
; CHECK-NEXT: [2] %for.body {4294967295,4294967295} [1]
; CHECK-NEXT: [2] %if.end4 {4294967295,4294967295} [1]
; CHECK-NEXT: [3] %entry {4294967295,4294967295} [2]
; CHECK-NEXT: [2] %for.cond34 {4294967295,4294967295} [1]
; CHECK-NEXT: [3] %for.cond16 {4294967295,4294967295} [2]
; CHECK-NEXT: Roots: %for.body %for.cond34
; CHECK-NEXT: PostDominatorTree for function: bar
; CHECK-NOT: <badref>

Expand Down
Loading
Loading