[Analysis][EphemeralValuesAnalysis][NFCI] Remove EphemeralValuesCache class #132454
Conversation
… class This is a follow-up to llvm#130210. The EphemeralValuesAnalysis pass used to return an EphemeralValuesCache object which used to hold the ephemeral values and used to provide a lazy collection of the ephemeral values, and an invalidation using the `clear()` function. This patch removes the EphemeralValuesCache class completely and instead returns the SmallVector containing the ephemeral values.
llvmbot
added
llvm:analysis
|
@llvm/pr-subscribers-llvm-transforms @llvm/pr-subscribers-llvm-analysis Author: vporpo (vporpo) ChangesThis is a follow-up to #130210. The EphemeralValuesAnalysis pass used to return an EphemeralValuesCache object which used to hold the ephemeral values and used to provide a lazy collection of the ephemeral values, and an invalidation using the This patch removes the EphemeralValuesCache class completely and instead returns the SmallVector containing the ephemeral values. Full diff: https://github.com/llvm/llvm-project/pull/132454.diff 7 Files Affected:
diff --git a/llvm/include/llvm/Analysis/EphemeralValuesCache.h b/llvm/include/llvm/Analysis/EphemeralValuesCache.h
index 2b50de9d22259..5e9664e00d961 100644
--- a/llvm/include/llvm/Analysis/EphemeralValuesCache.h
+++ b/llvm/include/llvm/Analysis/EphemeralValuesCache.h
@@ -23,35 +23,13 @@ class Function;
class AssumptionCache;
class Value;
-/// A cache of ephemeral values within a function.
-class EphemeralValuesCache {
- SmallPtrSet<const Value *, 32> EphValues;
- Function &F;
- AssumptionCache &AC;
- bool Collected = false;
-
- void collectEphemeralValues();
-
-public:
- EphemeralValuesCache(Function &F, AssumptionCache &AC) : F(F), AC(AC) {}
- void clear() {
- EphValues.clear();
- Collected = false;
- }
- const SmallPtrSetImpl<const Value *> &ephValues() {
- if (!Collected)
- collectEphemeralValues();
- return EphValues;
- }
-};
-
class EphemeralValuesAnalysis
: public AnalysisInfoMixin<EphemeralValuesAnalysis> {
friend AnalysisInfoMixin<EphemeralValuesAnalysis>;
static AnalysisKey Key;
public:
- using Result = EphemeralValuesCache;
+ using Result = SmallPtrSet<const Value *, 32>;
Result run(Function &F, FunctionAnalysisManager &FAM);
};
diff --git a/llvm/include/llvm/Analysis/InlineCost.h b/llvm/include/llvm/Analysis/InlineCost.h
index 90ee75773957a..8a51f6cc356cf 100644
--- a/llvm/include/llvm/Analysis/InlineCost.h
+++ b/llvm/include/llvm/Analysis/InlineCost.h
@@ -15,6 +15,7 @@
#include "llvm/ADT/APInt.h"
#include "llvm/ADT/STLFunctionalExtras.h"
+#include "llvm/Analysis/EphemeralValuesCache.h"
#include "llvm/Analysis/InlineModelFeatureMaps.h"
#include "llvm/IR/PassManager.h"
#include <cassert>
@@ -31,7 +32,6 @@ class Function;
class ProfileSummaryInfo;
class TargetTransformInfo;
class TargetLibraryInfo;
-class EphemeralValuesCache;
namespace InlineConstants {
// Various thresholds used by inline cost analysis.
@@ -280,23 +280,24 @@ InlineCost getInlineCost(
function_ref<const TargetLibraryInfo &(Function &)> GetTLI,
function_ref<BlockFrequencyInfo &(Function &)> GetBFI = nullptr,
ProfileSummaryInfo *PSI = nullptr, OptimizationRemarkEmitter *ORE = nullptr,
- function_ref<EphemeralValuesCache &(Function &)> GetEphValuesCache =
- nullptr);
+ function_ref<EphemeralValuesAnalysis::Result &(Function &)>
+ GetEphValuesCache = nullptr);
/// Get an InlineCost with the callee explicitly specified.
/// This allows you to calculate the cost of inlining a function via a
/// pointer. This behaves exactly as the version with no explicit callee
/// parameter in all other respects.
//
-InlineCost getInlineCost(
- CallBase &Call, Function *Callee, const InlineParams &Params,
- TargetTransformInfo &CalleeTTI,
- function_ref<AssumptionCache &(Function &)> GetAssumptionCache,
- function_ref<const TargetLibraryInfo &(Function &)> GetTLI,
- function_ref<BlockFrequencyInfo &(Function &)> GetBFI = nullptr,
- ProfileSummaryInfo *PSI = nullptr, OptimizationRemarkEmitter *ORE = nullptr,
- function_ref<EphemeralValuesCache &(Function &)> GetEphValuesCache =
- nullptr);
+InlineCost
+getInlineCost(CallBase &Call, Function *Callee, const InlineParams &Params,
+ TargetTransformInfo &CalleeTTI,
+ function_ref<AssumptionCache &(Function &)> GetAssumptionCache,
+ function_ref<const TargetLibraryInfo &(Function &)> GetTLI,
+ function_ref<BlockFrequencyInfo &(Function &)> GetBFI = nullptr,
+ ProfileSummaryInfo *PSI = nullptr,
+ OptimizationRemarkEmitter *ORE = nullptr,
+ function_ref<EphemeralValuesAnalysis::Result &(Function &)>
+ GetEphValuesCache = nullptr);
/// Returns InlineResult::success() if the call site should be always inlined
/// because of user directives, and the inlining is viable. Returns
diff --git a/llvm/lib/Analysis/EphemeralValuesCache.cpp b/llvm/lib/Analysis/EphemeralValuesCache.cpp
index 8b68df46950e1..f3cf655beef55 100644
--- a/llvm/lib/Analysis/EphemeralValuesCache.cpp
+++ b/llvm/lib/Analysis/EphemeralValuesCache.cpp
@@ -12,17 +12,14 @@
namespace llvm {
-void EphemeralValuesCache::collectEphemeralValues() {
- CodeMetrics::collectEphemeralValues(&F, &AC, EphValues);
- Collected = true;
-}
-
AnalysisKey EphemeralValuesAnalysis::Key;
-EphemeralValuesCache
+EphemeralValuesAnalysis::Result
EphemeralValuesAnalysis::run(Function &F, FunctionAnalysisManager &FAM) {
auto &AC = FAM.getResult<AssumptionAnalysis>(F);
- return EphemeralValuesCache(F, AC);
+ SmallPtrSet<const Value *, 32> EphValues;
+ CodeMetrics::collectEphemeralValues(&F, &AC, EphValues);
+ return EphValues;
}
} // namespace llvm
diff --git a/llvm/lib/Analysis/InlineCost.cpp b/llvm/lib/Analysis/InlineCost.cpp
index df212eb31950d..76c3dd8bb418a 100644
--- a/llvm/lib/Analysis/InlineCost.cpp
+++ b/llvm/lib/Analysis/InlineCost.cpp
@@ -271,7 +271,8 @@ class CallAnalyzer : public InstVisitor<CallAnalyzer, bool> {
CallBase &CandidateCall;
/// Getter for the cache of ephemeral values.
- function_ref<EphemeralValuesCache &(Function &)> GetEphValuesCache = nullptr;
+ function_ref<EphemeralValuesAnalysis::Result &(Function &)>
+ GetEphValuesCache = nullptr;
/// Extension points for handling callsite features.
// Called before a basic block was analyzed.
@@ -515,8 +516,8 @@ class CallAnalyzer : public InstVisitor<CallAnalyzer, bool> {
function_ref<const TargetLibraryInfo &(Function &)> GetTLI = nullptr,
ProfileSummaryInfo *PSI = nullptr,
OptimizationRemarkEmitter *ORE = nullptr,
- function_ref<EphemeralValuesCache &(Function &)> GetEphValuesCache =
- nullptr)
+ function_ref<EphemeralValuesAnalysis::Result &(Function &)>
+ GetEphValuesCache = nullptr)
: TTI(TTI), GetAssumptionCache(GetAssumptionCache), GetBFI(GetBFI),
GetTLI(GetTLI), PSI(PSI), F(Callee), DL(F.getDataLayout()), ORE(ORE),
CandidateCall(Call), GetEphValuesCache(GetEphValuesCache) {}
@@ -1133,8 +1134,8 @@ class InlineCostCallAnalyzer final : public CallAnalyzer {
ProfileSummaryInfo *PSI = nullptr,
OptimizationRemarkEmitter *ORE = nullptr, bool BoostIndirect = true,
bool IgnoreThreshold = false,
- function_ref<EphemeralValuesCache &(Function &)> GetEphValuesCache =
- nullptr)
+ function_ref<EphemeralValuesAnalysis::Result &(Function &)>
+ GetEphValuesCache = nullptr)
: CallAnalyzer(Callee, Call, TTI, GetAssumptionCache, GetBFI, GetTLI, PSI,
ORE, GetEphValuesCache),
ComputeFullInlineCost(OptComputeFullInlineCost ||
@@ -2794,7 +2795,7 @@ InlineResult CallAnalyzer::analyze() {
SmallPtrSet<const Value *, 32> EphValuesStorage;
const SmallPtrSetImpl<const Value *> *EphValues = &EphValuesStorage;
if (GetEphValuesCache)
- EphValues = &GetEphValuesCache(F).ephValues();
+ EphValues = &GetEphValuesCache(F);
else
CodeMetrics::collectEphemeralValues(&F, &GetAssumptionCache(F),
EphValuesStorage);
@@ -2980,7 +2981,8 @@ InlineCost llvm::getInlineCost(
function_ref<const TargetLibraryInfo &(Function &)> GetTLI,
function_ref<BlockFrequencyInfo &(Function &)> GetBFI,
ProfileSummaryInfo *PSI, OptimizationRemarkEmitter *ORE,
- function_ref<EphemeralValuesCache &(Function &)> GetEphValuesCache) {
+ function_ref<EphemeralValuesAnalysis::Result &(Function &)>
+ GetEphValuesCache) {
return getInlineCost(Call, Call.getCalledFunction(), Params, CalleeTTI,
GetAssumptionCache, GetTLI, GetBFI, PSI, ORE,
GetEphValuesCache);
@@ -3104,7 +3106,8 @@ InlineCost llvm::getInlineCost(
function_ref<const TargetLibraryInfo &(Function &)> GetTLI,
function_ref<BlockFrequencyInfo &(Function &)> GetBFI,
ProfileSummaryInfo *PSI, OptimizationRemarkEmitter *ORE,
- function_ref<EphemeralValuesCache &(Function &)> GetEphValuesCache) {
+ function_ref<EphemeralValuesAnalysis::Result &(Function &)>
+ GetEphValuesCache) {
auto UserDecision =
llvm::getAttributeBasedInliningDecision(Call, Callee, CalleeTTI, GetTLI);
diff --git a/llvm/lib/Transforms/IPO/Inliner.cpp b/llvm/lib/Transforms/IPO/Inliner.cpp
index 6b819a447bd77..d2eeb4468ea32 100644
--- a/llvm/lib/Transforms/IPO/Inliner.cpp
+++ b/llvm/lib/Transforms/IPO/Inliner.cpp
@@ -391,7 +391,8 @@ PreservedAnalyses InlinerPass::run(LazyCallGraph::SCC &InitialC,
}
// TODO: Shouldn't we be invalidating all analyses on F here?
// The caller was modified, so invalidate Ephemeral Values.
- FAM.getResult<EphemeralValuesAnalysis>(F).clear();
+ FAM.invalidate(
+ F, PreservedAnalyses::all().abandon(EphemeralValuesAnalysis::ID()));
DidInline = true;
InlinedCallees.insert(&Callee);
diff --git a/llvm/test/Transforms/Inline/cgscc-incremental-invalidate.ll b/llvm/test/Transforms/Inline/cgscc-incremental-invalidate.ll
index bc31bca614a40..7443581cf26b1 100644
--- a/llvm/test/Transforms/Inline/cgscc-incremental-invalidate.ll
+++ b/llvm/test/Transforms/Inline/cgscc-incremental-invalidate.ll
@@ -12,18 +12,15 @@
; CHECK: Invalidating analysis: LoopAnalysis on test1_f
; CHECK: Invalidating analysis: BranchProbabilityAnalysis on test1_f
; CHECK: Invalidating analysis: BlockFrequencyAnalysis on test1_f
-; CHECK: Invalidating analysis: EphemeralValuesAnalysis on test1_f
; CHECK: Running analysis: DominatorTreeAnalysis on test1_g
; CHECK: Invalidating analysis: DominatorTreeAnalysis on test1_g
; CHECK: Invalidating analysis: LoopAnalysis on test1_g
; CHECK: Invalidating analysis: BranchProbabilityAnalysis on test1_g
; CHECK: Invalidating analysis: BlockFrequencyAnalysis on test1_g
-; CHECK: Invalidating analysis: EphemeralValuesAnalysis on test1_g
; CHECK: Invalidating analysis: DominatorTreeAnalysis on test1_h
; CHECK: Invalidating analysis: LoopAnalysis on test1_h
; CHECK: Invalidating analysis: BranchProbabilityAnalysis on test1_h
; CHECK: Invalidating analysis: BlockFrequencyAnalysis on test1_h
-; CHECK: Invalidating analysis: EphemeralValuesAnalysis on test1_h
; CHECK-NOT: Invalidating analysis:
; CHECK: Running pass: DominatorTreeVerifierPass on test1_g
; CHECK-NEXT: Running analysis: DominatorTreeAnalysis on test1_g
diff --git a/llvm/unittests/Analysis/EphemeralValuesCacheTest.cpp b/llvm/unittests/Analysis/EphemeralValuesCacheTest.cpp
index c926cf97fcd69..e66c29a2cf4e8 100644
--- a/llvm/unittests/Analysis/EphemeralValuesCacheTest.cpp
+++ b/llvm/unittests/Analysis/EphemeralValuesCacheTest.cpp
@@ -8,9 +8,11 @@
#include "llvm/Analysis/EphemeralValuesCache.h"
#include "llvm/Analysis/AssumptionCache.h"
+#include "llvm/Analysis/TargetTransformInfo.h"
#include "llvm/AsmParser/Parser.h"
#include "llvm/IR/LLVMContext.h"
#include "llvm/IR/Module.h"
+#include "llvm/IR/PassInstrumentation.h"
#include "llvm/Support/SourceMgr.h"
#include "gmock/gmock-matchers.h"
#include "gtest/gtest.h"
@@ -46,8 +48,6 @@ define void @foo(i8 %arg0, i8 %arg1) {
)IR");
Function *F = M->getFunction("foo");
auto *BB = &*F->begin();
- AssumptionCache AC(*F);
- EphemeralValuesCache EVC(*F, AC);
auto It = BB->begin();
auto *C0 = &*It++;
auto *Assume0 = &*It++;
@@ -55,20 +55,21 @@ define void @foo(i8 %arg0, i8 %arg1) {
auto *C1 = &*It++;
auto *Assume1 = &*It++;
[[maybe_unused]] auto *Ret = &*It++;
- // Check emphemeral values.
- EXPECT_THAT(EVC.ephValues(),
- testing::UnorderedElementsAre(C0, Assume0, C1, Assume1));
- // Clear the cache and try again.
- EVC.clear();
- EXPECT_THAT(EVC.ephValues(),
- testing::UnorderedElementsAre(C0, Assume0, C1, Assume1));
- // Modify the IR, clear cache and recompute.
+ // Check ephemeral values.
+ FunctionAnalysisManager FAM;
+ FAM.registerPass([] { return EphemeralValuesAnalysis(); });
+ FAM.registerPass([] { return PassInstrumentationAnalysis(); });
+ FAM.registerPass([] { return AssumptionAnalysis(); });
+ FAM.registerPass([] { return TargetIRAnalysis(); });
+ auto Result = FAM.getResult<EphemeralValuesAnalysis>(*F);
+ EXPECT_THAT(Result, testing::UnorderedElementsAre(C0, Assume0, C1, Assume1));
+ // Modify the IR, invalidate and recompute.
Assume1->eraseFromParent();
C1->eraseFromParent();
- EXPECT_THAT(EVC.ephValues(),
- testing::UnorderedElementsAre(C0, Assume0, C1, Assume1));
- EVC.clear();
- EXPECT_THAT(EVC.ephValues(), testing::UnorderedElementsAre(C0, Assume0));
+ FAM.invalidate(
+ *F, PreservedAnalyses::all().abandon(EphemeralValuesAnalysis::ID()));
+ EXPECT_THAT(FAM.getResult<EphemeralValuesAnalysis>(*F),
+ testing::UnorderedElementsAre(C0, Assume0));
}
} // namespace
|
|
I agree the unittest is not super useful anymore, but it's a sanity check that the EphemeralValuesAnalysis does return the expected values i.e., they get collected by |
|
it still seems unnecessary to me, but keeping it is fine |
|
This introduced a compile-time regression (unlike the original change): https://llvm-compile-time-tracker.com/compare.php?from=c0b2c10e9f3a939c227a26aec3ba377f7cc25667&to=4adefcfb856aa304b7b0a9de1eec1814f3820e83&stat=instructions:u |
|
Good catch @nikic , I did notice a small compile-time increase compared to the previous one on the workload that was triggering the quadratic behavior but I assumed it was system noise, but now that I look into it I do see that the InlinerPass is 25% slower than before (but still way faster than without the Ephemeral Values Caching). I will revert the patch to investigate it further. |
|
After some investigation, it turns out the ~20% compile-time regression for the InlinerPass was actually due to run-to-run variance. In some runs the InlinerPass takes ~56s while in others it takes ~67s using the same clang binary. The only major functional difference between this and the previous version that I can think of is that in this one we invalidate the In Same for The only way this makes sense is if the individual call to |
|
I don't think |
|
ah sorry, I was looking at the wrong |
|
if it is the extra call to this patch doesn't change the number of analyses though right? we still have |
This is the most likely explanation, but I was expecting an equal increase in time spent inside
Yes it's added in both cases so the overhead should be similar in both cases, except if invalidating analyses more often somehow causes invalidate to run slower for some reason, though this seems highly unlikely. It seems that the perf stats I have collected are way too noisy, so I am not sure we can rely on them too much. |
|
I commented out the new call to So the ~0.05% diff in SPASS seems to be real. |
This is a follow-up to #130210. The EphemeralValuesAnalysis pass used to return an EphemeralValuesCache object which used to hold the ephemeral values and used to provide a lazy collection of the ephemeral values, and an invalidation using the
clear()function.This patch removes the EphemeralValuesCache class completely and instead returns the SmallVector containing the ephemeral values.