This is an archive of the discontinued LLVM Phabricator instance.

Improve LegacyPassManager API to correctly report modified status
ClosedPublic

Authored by serge-sans-paille on Jun 5 2020, 12:48 AM.

Details

Summary

Related to https://reviews.llvm.org/D80916

When calling on-the-fly passes from the legacy pass manager, the modification status is not reported, which is a problem in case we depend on an acutal transformation pass, and not only analyse.

Update the Legacy PM API to optionally report the changed status, assert if a change is detected but this change is lost.

Use that API for LoopExtractor where it's mandatory to pass the validation.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptJun 5 2020, 12:48 AM
ekatz added a comment.Jun 5 2020, 3:00 AM

There seems to be a more profound bug underneath.
Is this how every pass that use LoopInfo handle it?
It seems like the real issue is with LoopInfo - an analysis shouldn't do a transformation.

It seems like the real issue is with LoopInfo - an analysis shouldn't do a transformation.

Agreed. I tried to find out where LoopInfoWrapperPass would do this but I didn't spot it right away. @serge-sans-paille do you know where the modification happens or do you happen to have the reproducer?

Agreed. I tried to find out where LoopInfoWrapperPass would do this but I didn't spot it right away. @serge-sans-paille do you know where the modification happens or do you happen to have the reproducer?

I investigated, and BreakCriticalEdges is a dependency of LoopExtractor, and it's the pass that triggers the change. I supsect LoopSimplify, another analysis dep, has the same impact.
I don't understand where passes marked as dependency register their change status...

It looks like a limitation of the PM to me, I wonder what's the best approach there... Maybe just a big inline comment, that's gonna be removed once LegacyPM goes out?

ekatz added a comment.Jun 10 2020, 1:08 AM

I investigated, and BreakCriticalEdges is a dependency of LoopExtractor, and it's the pass that triggers the change.

This makes more sense. Every pass (in our case - BreakCriticalEdges) is responsible for reporting to the PM about the preserved (and non preserved) analyses.
I am not sure, what bug are you trying to tackle?
In case the LoopExtractor does nothing, it will report that all analyses are preserved, but the preceding call to BreakCriticalEdges will (and if it doesn't - it should) already report the non preserved analyses.
I guess, what you didn't expect is the fact that running the LoopExtractor will trigger BreakCriticalEdges, but although it may be a strange/ugly behavior, it is still valid.

Maybe a better implementation will be - calling BreakCriticalEdges as a utility from inside LoopExtractor. Consider what should be done to adapt this pass to the new PM?

In case the LoopExtractor does nothing, it will report that all analyses are preserved, but the preceding call to BreakCriticalEdges will (and if it doesn't - it should) already report the non preserved analyses.

This is the critical part. Does BreakCriticalEdges report a modification, if so the PM should be aware and it might be a PM bug, if not it is a BreakCriticalEdges bug. The user, here LoopExtractor, doesn't modify, so it should not check if a requirement did (as that is in general impossible).

Try another approach and fix the fact that the return status of a lazy transformation is simply ignored.

The legacy pass manager on-the-fly computation isn't meant to report status, which is the origin of the error this patch is trying to fix. Make that more explicit through member function renaming.

foad added a comment.Jun 18 2020, 7:04 AM

Makes sense to me, but I don't understand what the implications might be. For example, this pass will now unconditionally run a LoopSimplify pass. Previously could it have relied on an earlier LoopSimplify run, if all the intervening passes "preserved" LoopSimplify?

To put it another way: does this patch slow down the compiler? And/or does it add more passes to the list reported by -debug-pass=Structure, for a typical compile?

Improve the approach by updating the pass manager API to allow to get the pass return status when computing a Lazy analysis.

Improve the approach by updating the pass manager API to allow to get the pass return status when computing a Lazy analysis.

@foad / @jdoerfert if the approach looks fine, I'll update other passes that have the same issue.

jdoerfert accepted this revision.Jun 18 2020, 11:11 AM

LGTM. Thanks for this general solution to the problem :)

This revision is now accepted and ready to land.Jun 18 2020, 11:11 AM
fhahn added a subscriber: fhahn.Jun 18 2020, 12:51 PM

It would be good to explain the pass manager changes in the description (or maybe even better to submit separately)

ekatz requested changes to this revision.Jun 18 2020, 12:51 PM

This change should be split into 2.
One for the PM changes and the other for the LoopExtractor (that depend on the previous change).
Regardless, the changes in the PM should be reviewed by more people...

This revision now requires changes to proceed.Jun 18 2020, 12:51 PM

This change should be split into 2.
One for the PM changes and the other for the LoopExtractor (that depend on the previous change).

Fair.

Regardless, the changes in the PM should be reviewed by more people...

Do you have someone in mind? Or would you just want to wait?

Do you have someone in mind? Or would you just want to wait?

Not sure if waiting would help much, if we don't add new reviewers. Perhaps add @chandlerc , @rnk , @lattner , @mehdi_amini, @asbirlea, and maybe others who worked on these files (if needed).

Do you have someone in mind? Or would you just want to wait?

Not sure if waiting would help much, if we don't add new reviewers.

That was the reason I asked ;)

Perhaps add @chandlerc , @rnk , @lattner , @mehdi_amini, @asbirlea, and maybe others who worked on these files (if needed).

lattner resigned from this revision.Jun 18 2020, 9:43 PM
ychen added a subscriber: ychen.Jun 18 2020, 10:16 PM
serge-sans-paille retitled this revision from Correctly report modified status for LoopExtractor to Improve LegacyPassManager API t correctly report modified status.Jun 18 2020, 10:54 PM
serge-sans-paille edited the summary of this revision. (Show Details)
serge-sans-paille retitled this revision from Improve LegacyPassManager API t correctly report modified status to Improve LegacyPassManager API to correctly report modified status.

@ekatz I've updated the review title description to better reflect what it actually does. I've kept the review as a single bug to better showcase the usage, but once accepted I'll split it in two commits.

@mehdi_amini any thoughts on that one?

mehdi_amini added inline comments.Jun 18 2020, 11:22 PM
llvm/include/llvm/Pass.h
216

Can you document these added arguments?
It isn't clear to me what are these about: I wouldn't expect an analysis to change the IR for example.

serge-sans-paille marked an inline comment as done.

Update docstring

llvm/include/llvm/Pass.h
216

Will do. That's all the salt of this review: an analysis may trigger, as part of its dependencies, a pass that modifies the IR. This modification is currently ignored when computing on-the-fly passes.

mehdi_amini added inline comments.Jun 19 2020, 1:53 AM
llvm/include/llvm/Pass.h
216

Do you have examples of it? (if so maybe add them to the comment)

serge-sans-paille marked an inline comment as done.Jun 19 2020, 4:15 AM
serge-sans-paille added inline comments.
llvm/include/llvm/Pass.h
216

Yeah, LoopExtractor depends on BreakCriticalEdges. I'll add that to the comment.

Update docstring according to reviews

serge-sans-paille marked an inline comment as done.Jun 19 2020, 4:43 AM
nikic added inline comments.Jun 20 2020, 3:29 AM
llvm/include/llvm/IR/LegacyPassManagers.h
333–334

Just wondering, why std::tuple rather than std::pair?

serge-sans-paille marked 2 inline comments as done.Jun 21 2020, 3:18 AM
serge-sans-paille added inline comments.
llvm/include/llvm/IR/LegacyPassManagers.h
333–334

Because I find std::get<bool>(PassStatus) easier to read than std::get<1>(PassStatus) or PassStatus.second. No strong opinion here though :-)

mehdi_amini added inline comments.Jun 21 2020, 11:54 AM
llvm/include/llvm/Pass.h
216

Is this intended to work though? What about the new pass manager? Do we support analysis depending on IR transformations?

jdoerfert added inline comments.Jun 21 2020, 2:25 PM
llvm/include/llvm/Pass.h
216

LoopExtractor is not an analysis but a transformation. In lib/Analysis there is no occurrence of addRequiredID, in lib/Transforms there are 11 files. This is the common way to depend on IR transformations (I'd argue it is intended to work). The need in the new PM is reduced as most of these things depend on loop-simplify and lcssa, both provided implicitly by the new PM loop wrapper pass (I think). The other uses cases seem to not have a new PM implementation yet.

serge-sans-paille marked 2 inline comments as done.Jun 22 2020, 12:03 AM
serge-sans-paille added inline comments.

Up? This looks in pretty good shape to me.

foad added a comment.Jun 23 2020, 12:44 AM

If we're happy that getAnalysis can cause changes, and we don't want to update all callers of getAnalysis to expect a std::pair<AnalysisType &, bool> result, then your changes look good to me.

llvm/include/llvm/PassAnalysisSupport.h
266–269

How about std::tie(Pass, Changed) = findImplPass(...) to avoid the use of std::get ?

273

I'd find else assert(!std::get<bool>(PassStatus) && "...") easier to understand.

jdoerfert added a comment.EditedJun 23 2020, 7:20 AM

If we're happy that getAnalysis can cause changes,

I think this is the existing behavior and I'm reluctant to define it after years to be invalid without a really good reason (and it's the old PM so it might not be worth it).

and we don't want to update all callers of getAnalysis to expect a std::pair<AnalysisType &, bool> result, then your changes look good to me.

If we assume >95% not using the ID version to get a transformation, I think this is less churn for upstream and downstream users.

I think this is the existing behavior and I'm reluctant to define it after years to be invalid without a really good reason (and it's the old PM so it might not be worth it).

It is the existing behavior and it's actively used.

and we don't want to update all callers of getAnalysis to expect a std::pair<AnalysisType &, bool> result, then your changes look good to me.

If we assume >95% not using the ID version to get a transformation, I think this is less churn for upstream and downstream users.

That was my initial idea.

foad accepted this revision.Jun 24 2020, 3:14 AM

@ekatz any thoughts on this version?

ekatz added inline comments.Jun 24 2020, 12:10 PM
llvm/lib/Transforms/IPO/LoopExtractor.cpp
135

Maybe I am missing something, but isn't LoopExtractor dependent on BreakCriticalEdges? I mean, what are the transformations that the LoopInfo analysis depends on?

jdoerfert added inline comments.Jun 24 2020, 12:44 PM
llvm/lib/Transforms/IPO/LoopExtractor.cpp
135

what are the transformations that the LoopInfo analysis depends on?

No transformations:

void LoopInfoWrapperPass::getAnalysisUsage(AnalysisUsage &AU) const {                                                                                                                                              
  AU.setPreservesAll();
  AU.addRequiredTransitive<DominatorTreeWrapperPass>();
}
ekatz added inline comments.Jun 25 2020, 12:58 AM
llvm/lib/Transforms/IPO/LoopExtractor.cpp
135

So why would the Changed ever be set to true?

serge-sans-paille marked 2 inline comments as done.Jun 25 2020, 2:34 AM
serge-sans-paille added inline comments.
llvm/lib/Transforms/IPO/LoopExtractor.cpp
135

It's set to True because of the on the fly pass evaluation: asking for LoopInfo triggers the whole dependencies, including BreakCriticalEdges. That's why the API change I propose only impacts on the fly passes.

ekatz accepted this revision.Jun 25 2020, 9:44 AM

I admit, I don't like this change. This is a work around a design flaw in the old PM. However, it seems like the right decision to handle it, right now.
I think we should post a RFC for a new - *cough* - PM... ;)

LGTM, but please separate the patch into two: the PM related; and the LoopExtractor.

llvm/lib/Transforms/IPO/LoopExtractor.cpp
135

I see.

This revision is now accepted and ready to land.Jun 25 2020, 9:44 AM
mehdi_amini added inline comments.Jun 25 2020, 3:50 PM
llvm/include/llvm/Pass.h
216

. This is the common way to depend on IR transformations (I'd argue it is intended to work)

Right, so my memory of the old pass manager is that you indeed declare dependencies ahead of time, like the comment Serge is pointing at:

AU.addRequiredID(BreakCriticalEdgesID);

However what we have here is that after a transformation started (LoopExtractor::runOnModule(Module &M)) we need an *analysis* and we call getAnalysis.

It seems strange to me that a call to getAnalysis is *transforming* the IR in the middle of the pass here.

mehdi_amini added inline comments.Jun 25 2020, 3:57 PM
llvm/lib/Transforms/IPO/LoopExtractor.cpp
135

What is "on the fl pass evaluation" in this context? I'm missing why BreakCriticalEdges isn't triggered before LoopExtractor::runOnFunction invocation?

jdoerfert added inline comments.Jun 25 2020, 7:45 PM
llvm/include/llvm/Pass.h
216

It seems strange to me that a call to getAnalysis is *transforming* the IR in the middle of the pass here.

I'm not saying it is particularly sane. TBH, I *really hope* this is a temporary fix for an end-of-life part of the code base. Requiring major changes to the old-PM seems both unreasonable and dangerous too me. Just ignoring this is also not a great option. This patch gives us a reasonable way to deal with this "feature" of the old-PM.

ekatz added inline comments.Jun 26 2020, 4:13 AM
llvm/lib/Transforms/IPO/LoopExtractor.cpp
135

To add to your question, just to make sure I understand it correctly- if getAnalysis was never called, then BreakCriticalEdges wasn't either?

This revision was automatically updated to reflect the committed changes.
serge-sans-paille marked an inline comment as done.
serge-sans-paille marked an inline comment as done.Jun 26 2020, 6:03 AM
serge-sans-paille added inline comments.
llvm/lib/Transforms/IPO/LoopExtractor.cpp
135

That's what I observed, yes.

Meinersbur added inline comments.
llvm/lib/Transforms/IPO/LoopExtractor.cpp
135

@mehdi_amini "On the fly" is a workaround for the legacy pass manager where coarser grain passes (here: ModulePass) cannot depend on finer-grain passes (here: FunctionPass).

Usually, the legacy pass manager would call all dependency passes before the pass itself, such that dependency pass'es runOnFunction is called be the PassManager, and receives the Changed status of that dependency.
When a ModulePass requests a FunctionPass analysis, the PassManager will not have called it in advance, but it will call it during getAnalsysis(). If that analysis modifies the IR, the changed status was not propagated up the PassManager stack, in this case the MPPassManager.

Since the getOnTheFlyPass is actually a method of MPPassManager, Changed in MPPassManager::runOnModule could also me made a field which getOnTheFlyPass sets to true if FPP->run(F) returns and we would not have to propagate the Changes status though method arguments.

@serge-sans-paille Did you actually consider such an alternative design?

serge-sans-paille marked an inline comment as done.Jun 26 2020, 12:10 PM
serge-sans-paille added inline comments.
llvm/lib/Transforms/IPO/LoopExtractor.cpp
135

@Meinersbur the whole *Changed* stuff is not state-based, so I naturally choose that path. And I'd favor it anyway, as it's more explicit. But that's not a strong opinion.

FWIW, I prefer this strongly over a "state" approach if everything else is equal.

plotfi added a subscriber: plotfi.Jun 26 2020, 4:43 PM

Just an FYI, PassAnalysisSupport.h is missing an #include <tuple>. Landed the addition in https://github.com/llvm/llvm-project/commit/c918c1a91a03f612c1b1e81b82460fbf80fc592a

mehdi_amini added inline comments.Jun 26 2020, 4:52 PM
llvm/lib/Transforms/IPO/LoopExtractor.cpp
135

Thanks @Meinersbur ; this is great explanation!
@serge-sans-paille can you capture this in the comment describing the getAnalysis() interface you changed? (or point to a doc if this is documented somewhere)

Also it wasn't obvious to me that this was a module pass (the method is called runOnFunction here, and I didn't check the declaration in the class to see that it wasn't an override).