This is an archive of the discontinued LLVM Phabricator instance.

[PM] Support invalidation of inner analysis managers from a pass over the outer IR unit.
ClosedPublic

Authored by chandlerc on Nov 29 2016, 3:01 AM.

Details

Summary

This never really got implemented, and was very hard to test before
a lot of the refactoring changes to make things more robust. But now we
can test it thoroughly and cleanly, especially at the CGSCC level.

The core idea is that when an inner analysis manager proxy receives the
invalidation event for the outer IR unit, it needs to walk the inner IR
units and propagate it to the inner analysis manager for each of those
units. For example, each function in the SCC needs to get an
invalidation event when the SCC gets one.

The function / module interaction is somewhat boring here. This really
becomes interesting in the face of analysis-backed IR units. This patch
effectively handles all of the CGSCC layer's needs -- both invalidating
SCC analysis and invalidating function analysis when an SCC gets
invalidated.

However, this second aspect doesn't really handle the
LoopAnalysisManager well at this point. That one will need some change
of design in order to fully integrate, because unlike the call graph,
the entire function behind a LoopAnalysis's results can vanish out from
under us, and we won't even have a cached API to access. I'd like to try
to separate solving the loop problems into a subsequent patch though in
order to keep this more focused so I've adapted them to the API and
updated the tests that immediately fail, but I've not added the level of
testing and validation at that layer that I have at the CGSCC layer.

An important aspect of this change is that the proxy for the
FunctionAnalysisManager at the SCC pass layer doesn't work like the
other proxies for an inner IR unit as it doesn't directly manage the
FunctionAnalysisManager and invalidation or clearing of it. This would
create an ever worsening problem of dual ownership of this
responsibility, split between the module-level FAM proxy and this
SCC-level FAM proxy. Instead, this patch changes the SCC-level FAM proxy
to work in terms of the module-level proxy and defer to it to handle
much of the updates. It only does SCC-specific invalidation. This will
become more important in subsequent patches that support more complex
invalidaiton scenarios.

Event Timeline

chandlerc updated this revision to Diff 79528.Nov 29 2016, 3:01 AM
chandlerc retitled this revision from to [PM] Support invalidation of inner analysis managers from a pass over the outer IR unit..
chandlerc updated this object.
chandlerc added a reviewer: jlebar.
chandlerc added a subscriber: llvm-commits.
silvas added a subscriber: silvas.Dec 1 2016, 12:24 AM

nice to see this feature finally implemented!

jlebar added inline comments.Dec 6 2016, 6:06 PM
include/llvm/Analysis/CGSCCPassManager.h
168

Perhaps we could be more concrete:

If the proxy analysis itself is preserved, then we assume that the set of SCCs in the Module hasn't changed. Thus any pointers to SCCs in the CGSCCAnalysisManager are still valid, and we don't need to call \c clear on the CGSCCAnalysisManager.

173

What's an "inner set"? (Maybe this whole parenthetical can go.)

179

I don't understand this last sentence. This is already a template specialization, so "this template" is...the template we're specializing? Does each specialization of InnerAnalysisManagerProxy::Result::invalidate need to specialize as described here?

189

garph

452

Suggest rephrasing "does not mirror the proxy" -- it's hard to understand what you mean.

Perhaps just remove this para.

461

Starting from "Instead" is very hard to parse. Perhaps something like:

Instead, this layer is only responsible for SCC-local invalidation events. We work with the module's FunctionAnalysisManager to invalidate function analyses.

include/llvm/IR/PassManager.h
781

FWIW I don't think that "containing IR unit" is particularly helpful.

In my patch that rewrites these comments, I changed it to talk about the "larger" and "smaller" IR units:

/// \brief An analysis over a "larger" IR unit that provides access to an analysis manager over a "smaller" IR unit.
class InnerAnalysisManagerProxy {
  [...]
  /// \brief Handler for invalidation of the "larger" IR unit, \c IRUnitT.
  bool invalidate(...) {}
}

But we can go over this separately.

797

This comment makes sense -- maybe we simply don't need the one I commented on earlier that didn't make sense.

lib/Analysis/CGSCCPassManager.cpp
95

Comma before "and".

98

"if it's unavailable"

106

"when it's created", although maybe it's good enough to say "we can observe the new call graph."

111

comma before "so".

112

Do we in fact do this if the call graph is about to become invalid? The comment on the first if statement in this functions ays we go into the if if "the call graph is going to be invalidated". Maybe this whole comment is stale?

122

Suggest s/Otherwise/Return/ because the thing we're contrasting against is somewhat far away.

148

Would it be correct to rephrase this to say

rely on the FAMMP to invalidate the function analyses.

? If so, maybe we can say that? If not, I do not understand what this is trying to say.

156

s/hard-code/special-case/?

172

Suggest

entire SCC layer, which includes this proxy, is cleared.

lib/Analysis/LoopPassManager.cpp
33
  • s/, there/, as there/
  • s/making/, making/

But maybe even better would be to rephrase a bit:

If this proxy isn't marked as preserved, the set of Function objects in the module may have changed. We therefore can't call InnerAM->invalidate(), because any pointers to Functions it has may be stale. Instead, we call clear().

unittests/Analysis/CGSCCPassManagerTest.cpp
127

Any reason not to take an std::function as the argument?

129

Not anymore!

535

Any reason not to write these as independent TESTs? As-is I have to read carefully to see that you reset the SCCAnalysisRuns counter between tests, and it's not obvious whether you're (intentionally or unintentionally) carrying any additional state between the various blocks.

616

s/&/and/ because don't we already have enough meanings for "&" in C++? :)

625

Raw string?

chandlerc updated this revision to Diff 80988.Dec 9 2016, 5:59 PM
chandlerc marked 19 inline comments as done.

Rebase and address code review comments.

Updated with fixes to hopefully everything (please let me know if I missed something). Responses in a few cases below.

include/llvm/Analysis/CGSCCPassManager.h
179

Sorry, I just kept a bunch of pointless documentation from the original template. Nuked this.

include/llvm/IR/PassManager.h
781

Sure, happy to try different sets of terminology and see what works best.

I can see why "containing" would be problematic. The code itself currently uses "inner" and "outer" which at least seem somewhat better and I've made the comments match that for now.

lib/Analysis/CGSCCPassManager.cpp
112

This comment is indeed stale.

unittests/Analysis/CGSCCPassManagerTest.cpp
127

It seems more idiomatic. In other, more interesting cases it can actually matter because we run out of implicit conversions. It also avoids spelling out the function type twice.

625

Would it be an improvement? I'm not sure...

Anyways, should be a separate patch and applied to all the textual modules here if we can rely on this feature and it is worth using.

jlebar accepted this revision.Dec 9 2016, 7:29 PM
jlebar edited edge metadata.

I am happy.

unittests/Analysis/CGSCCPassManagerTest.cpp
625

Personally I think it would be, if for no other reason than it's a lot easier to edit text inside of raw strings. (Also it's visually less noisy to read.)

But I'm fine doing that separately.

This revision is now accepted and ready to land.Dec 9 2016, 7:29 PM
This revision was automatically updated to reflect the committed changes.