[NewPM] LiveIntervals: Check dependencies for invalidation #123563
Conversation
|
@llvm/pr-subscribers-llvm-regalloc Author: Akshat Oke (optimisan) ChangesFull diff: https://github.com/llvm/llvm-project/pull/123563.diff 2 Files Affected:
diff --git a/llvm/include/llvm/CodeGen/LiveIntervals.h b/llvm/include/llvm/CodeGen/LiveIntervals.h
index 161bb247a0e968..540651ea114427 100644
--- a/llvm/include/llvm/CodeGen/LiveIntervals.h
+++ b/llvm/include/llvm/CodeGen/LiveIntervals.h
@@ -113,6 +113,9 @@ class LiveIntervals {
LiveIntervals(LiveIntervals &&) = default;
~LiveIntervals();
+ bool invalidate(MachineFunction &MF, const PreservedAnalyses &PA,
+ MachineFunctionAnalysisManager::Invalidator &Inv);
+
/// Calculate the spill weight to assign to a single instruction.
/// If \p PSI is provided the calculation is altered for optsize functions.
static float getSpillWeight(bool isDef, bool isUse,
diff --git a/llvm/lib/CodeGen/LiveIntervals.cpp b/llvm/lib/CodeGen/LiveIntervals.cpp
index f38527a3ce6a31..64c184f5553ab2 100644
--- a/llvm/lib/CodeGen/LiveIntervals.cpp
+++ b/llvm/lib/CodeGen/LiveIntervals.cpp
@@ -127,6 +127,20 @@ LiveIntervalsWrapperPass::LiveIntervalsWrapperPass() : MachineFunctionPass(ID) {
LiveIntervals::~LiveIntervals() { clear(); }
+bool LiveIntervals::invalidate(
+ MachineFunction &MF, const PreservedAnalyses &PA,
+ MachineFunctionAnalysisManager::Invalidator &Inv) {
+ auto PAC = PA.getChecker<LiveIntervalsAnalysis>();
+
+ if (!PAC.preserved() && !PAC.preservedSet<AllAnalysesOn<MachineFunction>>())
+ return true;
+
+ // LiveIntervals holds pointers to these results, so check for their
+ // invalidation.
+ return (Inv.invalidate<SlotIndexesAnalysis>(MF, PA) ||
+ Inv.invalidate<MachineDominatorTreeAnalysis>(MF, PA));
+}
+
void LiveIntervals::clear() {
// Free the live intervals themselves.
for (unsigned i = 0, e = VirtRegIntervals.size(); i != e; ++i)
|
| // invalidation. | ||
| return (Inv.invalidate<SlotIndexesAnalysis>(MF, PA) || | ||
| Inv.invalidate<MachineDominatorTreeAnalysis>(MF, PA)); | ||
| } |
There was a problem hiding this comment.
IIUC when control flow reaches here, it implies live intervals or all machine function analyses are preserved. It invalidates them forcibly. Move them to SlotIndexesAnalysis?
There was a problem hiding this comment.
invalidate() actually means isThisResultInvalid(IR, PA), so if SlotIndexes reports that it has been made invalid, this analysis should also be invalid. SI and MDT are good with the default invalidate() implementation since they don't depend on other analyses.
There was a problem hiding this comment.
Then
PreservedAnalyses SomePass::run(IRUnitT &, AnalysisManager &) {
PreservedAnalyses PA;
PA.preserve<LiveIntervals>();
return PA;
}will no longer work, which is counter-intuitive... because it doesn't preserve SlotIndexes explicitly.
There was a problem hiding this comment.
Yes, it is related here #123045 (comment)
New PM can't report such cases. Could add such a functionality behind a flag but I guess it's unlikely for more cases like this to exist, since they would lead to invalid reads (as the PM deletes the results).
There was a problem hiding this comment.
Then
PreservedAnalyses SomePass::run(IRUnitT &, AnalysisManager &) { PreservedAnalyses PA; PA.preserve<LiveIntervals>(); return PA; }will no longer work, which is counter-intuitive... because it doesn't preserve
SlotIndexesexplicitly.
if SlotIndexes is invalidated, then LiveIntervals may reference an invalid SlotIndexes, so if we kept around LiveIntervals it may be invalid to use. so invalidating it regardless makes sense.
There was a problem hiding this comment.
Yes, it is related here #123045 (comment)
New PM can't report such cases. Could add such a functionality behind a flag but I guess it's unlikely for more cases like this to exist, since they would lead to invalid reads (as the PM deletes the results).
I don't think it's easily trackable which analyses depend on other analyses without a bunch of extra infrastructure, so unless this is a problem that comes up a lot I'm inclined to say it's not worth the effort
| // invalidation. | ||
| return (Inv.invalidate<SlotIndexesAnalysis>(MF, PA) || | ||
| Inv.invalidate<MachineDominatorTreeAnalysis>(MF, PA)); | ||
| } |
There was a problem hiding this comment.
Then
PreservedAnalyses SomePass::run(IRUnitT &, AnalysisManager &) { PreservedAnalyses PA; PA.preserve<LiveIntervals>(); return PA; }will no longer work, which is counter-intuitive... because it doesn't preserve
SlotIndexesexplicitly.
if SlotIndexes is invalidated, then LiveIntervals may reference an invalid SlotIndexes, so if we kept around LiveIntervals it may be invalid to use. so invalidating it regardless makes sense.
| // invalidation. | ||
| return (Inv.invalidate<SlotIndexesAnalysis>(MF, PA) || | ||
| Inv.invalidate<MachineDominatorTreeAnalysis>(MF, PA)); | ||
| } |
There was a problem hiding this comment.
Yes, it is related here #123045 (comment)
New PM can't report such cases. Could add such a functionality behind a flag but I guess it's unlikely for more cases like this to exist, since they would lead to invalid reads (as the PM deletes the results).
I don't think it's easily trackable which analyses depend on other analyses without a bunch of extra infrastructure, so unless this is a problem that comes up a lot I'm inclined to say it's not worth the effort
No description provided.