This is an archive of the discontinued LLVM Phabricator instance.

[NFC][NewPM] Remove some AnalysisManager invalidate methods
ClosedPublic

Authored by aeubanks on Apr 14 2021, 6:43 PM.

Details

Summary

These were misleading, they're more of a "clear" than an "invalidate".

We shouldn't be individually clearing analysis results. Either we clear
all analyses when some IR becomes invalid, or we properly go through
invalidation.

There was only one use of this, which can be simulated with
AM.invalidate(F, PA).

Diff Detail

Event Timeline

aeubanks created this revision.Apr 14 2021, 6:43 PM
aeubanks requested review of this revision.Apr 14 2021, 6:43 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 14 2021, 6:43 PM

This does break https://reviews.llvm.org/D98103, but IMO that analysis shouldn't be immutable, but rather just a typical function analysis that can be invalidated like any other stateful function analysis result.

This does break https://reviews.llvm.org/D98103, but IMO that analysis shouldn't be immutable, but rather just a typical function analysis that can be invalidated like any other stateful function analysis result.

Right, that captures what we agreed would be desirable.

mtrofin added inline comments.Apr 14 2021, 7:28 PM
llvm/include/llvm/IR/PassManager.h
867

so is there a way then to tell the FAM to delete an immutable analysis, or nothing uses that?

one possible scenario may be in the case the analysis occupies too much memory, and there's a good place to dispose of it.

aeubanks added inline comments.Apr 14 2021, 8:04 PM
llvm/include/llvm/IR/PassManager.h
867

Immutable analyses are generally stateless so they should be fairly small.

There wouldn't be a way to tell the FAM to delete an immutable analysis, outside of clearing all analyses for some (invalidated) IR. But I think that should be fine.

mtrofin added inline comments.Apr 14 2021, 8:34 PM
llvm/include/llvm/IR/PassManager.h
867

Immutable analyses are generally stateless so they should be fairly small.

Profile info could be large, for instance.

There wouldn't be a way to tell the FAM to delete an immutable analysis, outside of clearing all analyses for some (invalidated) IR. But I think that should be fine.

We could rename instead the invalidate API to "clearResult"?

Ah yeah profile summary info could be large.

But it doesn't seem like memory usage is a big issue right now. We can always revive these methods later if we want to.

Ah yeah profile summary info could be large.

But it doesn't seem like memory usage is a big issue right now. We can always revive these methods later if we want to.

True. It's also possible for the immutable analysis to offer its own APIs for managing its state.

mtrofin accepted this revision.Apr 14 2021, 8:55 PM
This revision is now accepted and ready to land.Apr 14 2021, 8:55 PM
This revision was landed with ongoing or failed builds.Apr 15 2021, 4:57 PM
This revision was automatically updated to reflect the committed changes.