Skip to content

[NewPM] LiveIntervals: Check dependencies for invalidation #123563

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 2 commits into from
Jan 24, 2025
Merged

[NewPM] LiveIntervals: Check dependencies for invalidation #123563

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
02db00b
[NewPM] LiveIntervals: Check dependencies for invalidation
optimisan Jan 20, 2025
1549605
remove parenthesis
optimisan Jan 20, 2025
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
3 changes: 3 additions & 0 deletions llvm/include/llvm/CodeGen/LiveIntervals.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
14 changes: 14 additions & 0 deletions llvm/lib/CodeGen/LiveIntervals.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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).

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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


void LiveIntervals::clear() {
// Free the live intervals themselves.
for (unsigned i = 0, e = VirtRegIntervals.size(); i != e; ++i)
Expand Down
Loading