[PATCH] D23738: [PM] Extend the explicit 'invalidate' method API on analysis results to accept an Invalidator that allows them to invalidate themselves if their dependencies are in turn invalidated.

Chandler Carruth via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Nov 28 14:08:18 PST 2016


chandlerc marked 2 inline comments as done.
chandlerc added a comment.

Thanks so much! Submitting with suggested changes and getting the next patch in my queue ready for review.



================
Comment at: include/llvm/IR/PassManager.h:610
+      IMapI->second = Result.invalidate(IR, PA, Inv);
+    }
+
----------------
jlebar wrote:
> chandlerc wrote:
> > silvas wrote:
> > > jlebar wrote:
> > > > WRT the names:
> > > > 
> > > > I agree "invalidate" isn't a great name, because
> > > > 
> > > >   if (!my_pass->invalidate(...)) {
> > > >     // my_pass was not invalidated!
> > > >   }
> > > > 
> > > > `checkIfInvalidated`?  `onIRChange`?  `notifyIRChanged`?  I like the first one because the bool return value sort of makes sense.
> > > > 
> > > > This would also change the comment above -- we're not really "trying" to invalidate the result.  More informing it that something has changed, and giving it the option to mark itself as invalid.
> > > > 
> > > > I still don't really understand what we're supposed to do inside the `invalidate` function, so I'm not sure about the `Invalidator` class name.  The only example I have is
> > > > 
> > > >     bool TestIndirectFunctionAnalysis::invalidate(Function &F, const PreservedAnalyses &PA,
> > > >                     FunctionAnalysisManager::Invalidator &Inv) {
> > > >       return !PA.preserved<TestIndirectFunctionAnalysis>() ||
> > > >              Inv.invalidate<TestFunctionAnalysis>(F, PA);
> > > >     }
> > > > 
> > > > It naively seems to me that it would be better to structure things so that we never run this function if PA.preserved<TestIndirectFunctionAnalysis>().  The Invalidator could ensure this.  Then our function could be
> > > > 
> > > >   bool checkIfInvalidated(...) {
> > > >     return Inv.checkIfInvalidated<Dep1>() || Inv.checkIfInvalidated<Dep2>();
> > > >   }
> > > > 
> > > > which actually makes sense to me.  I think ideally Inv.checkIfInvalidated would take zero parameters, since it's just boilerplate, but maybe it should still take the Function/Module/whatever.  We'd at least be able to get rid of the PreservedAnalyses argument, I think?
> > > Currently, the `invalidate` method is a way to opt out of being invalidated (e.g. for TTI which knows it only depends on the target).
> > > 
> > > However, this cannot trigger invalidation of additional analysis result objects, which is what is needed when one analysis result holds pointers to another analysis results in order to avoid use-after free errors[1]. Hence the addition of the extra invalidation machinery in this patch. (this patch is by no means a comprehensive solution; I mentioned some problem areas in the initial comment in this review). 
> > > 
> > > The `Invalidator` that is added in this patch basically provides a constrained interface to the analysis result list so that analysis result objects can transitively find and check whether any analysis result objects that they hold a pointer to have been invalidated. This allows invalidation to propagate from dependencies to dependents and trigger additional invalidation, avoiding use-after-free. (for layering reasons, analyses cannot know what will depend on them, but only what they depend on, so to do this with object recursion requires this process to be top-down dependent-to-dependency relying on memoization, rather than bottom-up dependency-to-dependent with a direct traversal)
> > > 
> > > 
> > > Also, unfortunately, it is not entirely practical to avoid calling the `invalidate` method when `PA.preserved<FooAnalysis>()` is true. The difficulty again occurs when analysis result objects for FooAnalysis hold pointers to other analysis result objects. In that case, transformation passes would have to call `PA.preserve<BarAnalysis>()` for every analysis that FooAnalysis result objects transitively hold a pointer to (otherwise there would be a dangling pointer / use-after-free error when the BarAnalysis result object is invalidated due to not being in the "preserved" set). There is actually an additional difficulty due to `AliasAnalysis`, which depends on a dynamic (but fixed for a given pass pipeline invocation) set of type-erased analyses; hence there is no static set of `PA.preserve<...>()` calls which is necessarily correct.
> > > 
> > > I don't think that the `PreservedAnalyses` argument can be eliminated, because the `!PA.preserved<FooAnalysis>()` check is essentially the base-case of the object recursion facilitated by the `Invalidator`. I.e. it determines the roots from which invalidation must be propagated from dependency to dependent (although for layering reasons the object recursion goes from dependent to dependency, with memoization to keep the complexity under control).
> > > 
> > > [1] This is actually pervasive. Running any non-trivial pipeline on any non-trivial piece of code will currently trigger numerous use-after free errors with the new pass manager.
> > First and foremost, I'm down with renaming this, but I'd really like that to be a separate patch. It'll be a ton of boring changes that I don't want to try and juggle in this commit.
> > 
> > Second, regarding the preserved analysis argument and the IR unit argument: I think they both have good reasons to exist.
> > 1) Transformations may want to preserve an entire set of analyses, and analyses should query in the preserved analyses to see if a set that covers them has been preserved even if the individual analyses has not been.
> > 2) Analyses may want to check trivial information about the IR to determine from first principles that they have been preserved.
> > 
> > And as Sean points out, this is really just about adding the first of several pieces of infrastructure necessary to make invalidation reliable.
> > 
> > I'd like to brainstorm different names for this outside the review though to see if there is a clearly superior alternative.
> I think I understand why you want the three args; thanks Sean and Chandler for the explanation.
> 
> > I'd like to brainstorm different names for this outside the review though to see if there is a clearly superior alternative.
> 
> sgtm.  What is the right venue for this discussion?  We can seed it with my comment from above.
I'll chat with you and maybe others on IRC first to get some ideas, and then I'll send a dedicated patch to do this.


================
Comment at: include/llvm/IR/PassManager.h:357
+  /// Requires iterators to be valid across appending new entries and arbitrary
+  /// erases. Provides the pass ID to enable finding iterators to a given entry
+  /// in maps below, and provides the storage for the actual result concept.
----------------
jlebar wrote:
> Is "pass ID" still the right way to describe the first element of the pair?
switched to 'analysis ID' which seems more clear.


================
Comment at: include/llvm/IR/PassManager.h:385
+    template <typename PassT>
+    bool invalidate(IRUnitT &IR, const PreservedAnalyses &PA) {
+      AnalysisKey *ID = PassT::ID();
----------------
jlebar wrote:
> Should we comment what this returns?  It's not obvious to me from the name, and the body is nontrivial.
Yea, this was just completely missing documentation. Added. Happy to tweak and/or improve in follow-up commits.


https://reviews.llvm.org/D23738





More information about the llvm-commits mailing list