Page MenuHomePhabricator

[mlir] Pass AnalysisManager as optional parameter to analysis ctor, so it can request any other analysis as dependency
ClosedPublic

Authored by Hardcode84 on Apr 11 2021, 2:25 PM.

Diff Detail

Event Timeline

Hardcode84 created this revision.Apr 11 2021, 2:25 PM
Hardcode84 requested review of this revision.Apr 11 2021, 2:25 PM

fix getAnalysis

Templates fix

rriddle requested changes to this revision.Apr 12 2021, 5:56 PM

You'll also need to make sure that when an analysis is invalidated that all of the things that depend on it check to see if they also need to be invalidated. I would take a look at the Analysis Manager in LLVM as an inspiration for inter-analysis dependencies.

This revision now requires changes to proceed.Apr 12 2021, 5:56 PM

Thanks for taking a look at this, it has been pushed off for a while.

You'll also need to make sure that when an analysis is invalidated that all of the things that depend on it check to see if they also need to be invalidated. I would take a look at the Analysis Manager in LLVM as an inspiration for inter-analysis dependencies.

This should be already covered by isInvalidated (see AnalysisWithDependency::isInvalidated in tests). It is probably a good idea to have a template helper which can generate isInvalidated automatically, but its better make as separate patch.

You'll also need to make sure that when an analysis is invalidated that all of the things that depend on it check to see if they also need to be invalidated. I would take a look at the Analysis Manager in LLVM as an inspiration for inter-analysis dependencies.

This should be already covered by isInvalidated (see AnalysisWithDependency::isInvalidated in tests). It is probably a good idea to have a template helper which can generate isInvalidated automatically, but its better make as separate patch.

That only handles very simple cases, not the more complex. For example, say you have an analysis A that depends on an analysis B, which depends on analysis C. A -> B -> C. What happens if a pass marks analysis B as preserved but not C? It seems that B would be invalidated properly (because C is not preserved), but A would still see that B has been preserved so it wouldn't invalidate itself.

Properly handle analysis dependency chains

Added more comments, looking good.

mlir/include/mlir/Pass/AnalysisManager.h
64

Can you document this?

118–124

The method is already marked final, why add an override?

119

nit: Spell out to result

186

If we changed analyses to use a MapVector, can we rely on dependencies being ordered first to avoid the need for this? (MapVector guarantees deterministic iteration based on insertion time)

216

Please document these methods.

MapVector, comments

Hardcode84 marked an inline comment as done.Apr 14 2021, 3:51 PM
Hardcode84 added inline comments.
mlir/include/mlir/Pass/AnalysisManager.h
64

done. also moved to private for now

118–124

you are rigth

186

good idea, done

rriddle accepted this revision.Apr 16 2021, 10:26 AM

Great!

mlir/include/mlir/Pass/AnalysisManager.h
65

nit: Comments should have proper punctuation.

72

nit: Add a comment on why this is a friend.

117–118

Can you add it that we also unpreserve any analyses from pa that were invalidated? Maybe we should also rename this method to invalidate, given that it mutates as well.

185–188
This revision is now accepted and ready to land.Apr 16 2021, 10:26 AM

https://mlir.llvm.org/docs/PassManagement/#analysis-management
Docs probably also need to be updated and I will need some help with that. Should it go to separate review?

rriddle requested changes to this revision.Apr 16 2021, 10:53 AM

https://mlir.llvm.org/docs/PassManagement/#analysis-management
Docs probably also need to be updated and I will need some help with that. Should it go to separate review?

Let's roll that into this one. (Will mark as changes requested for now)

Can you also update the description?

This revision now requires changes to proceed.Apr 16 2021, 10:53 AM

review comments, docs

Hardcode84 edited the summary of this revision. (Show Details)Apr 16 2021, 12:40 PM

fix code in docs

rriddle accepted this revision.Apr 20 2021, 12:06 AM

LG thanks.

mlir/docs/PassManagement.md
160–166
175–178
209

Add comments here for the interesting bits.

920–925
This revision is now accepted and ready to land.Apr 20 2021, 12:06 AM

update docs

Hardcode84 marked 4 inline comments as done.Apr 20 2021, 4:39 AM