[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.

[Sean Silva via llvm-commits]

silvas added inline comments.


================
Comment at: include/llvm/IR/PassManager.h:610
+      IMapI->second = Result.invalidate(IR, PA, Inv);
+    }
+
----------------
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.


https://reviews.llvm.org/D23738