[llvm-dev] Should analyses be able to hold AssertingVH to IR? (related to PR28400)

[Chandler Carruth via llvm-dev]

Cool, moving forward with this as Sanjoy clarified on the patches that he's
also happy with this result and everyone else seems happy as well.

Thanks!
-Chandler

On Mon, Jan 23, 2017 at 10:07 PM Sean Silva <chisophugis at gmail.com> wrote:

> On Mon, Jan 23, 2017 at 1:08 AM, Chandler Carruth <chandlerc at gmail.com>
> wrote:
>
> This thread kinda died. I'd like to revive it.
>
> The new PM stuff is making excellent progress, and this is actually one of
> the last things to clean up.
>
> On Mon, Aug 8, 2016 at 1:10 AM Sean Silva <chisophugis at gmail.com> wrote:
>
> Thoughts? For the moment I have put in a workaround (r274457) that makes
> jump-threading invalidate LVI.
>
>
> Is everybody happy with this workaround?
>
>
> I wasn't too happy with it, but I had no better suggestion.
>
> As the infrastructure matured, what I think is a substantially less
> horrible workaround is available in the form of what I implemented in
> r292773. Instead of just working around this for each analysis, this works
> around it in GlobalDCE for *any* function analysis stashing an AssertingVH.
> The down side is that it only defends against *function* removal and
> *function* analyses. =[
>
> This may be a tiny bit better in some senses, but in others its worse, and
> frankly I think it is a pretty gross hack even in the best of cases. But
> let's take a look at some of the cases you identified:
>
> #1: CallGraph has an asserting VH on functions. But my workaround doesn't
> help at all, much to my surprise afterward! Why? Well of course because
> CallGraph is a *module analysis*. We can't just go invalidating every
> module analysis every time we remove a function... :: sigh ::
>
> #2: SCEV and LVI have *basic block* asserting VHs. For some reason, all
> the test cases I have stem from deleting an entire function, but there is
> no real reason that will be the case. It seems entirely plausible to nuke a
> basic block out from under one of these.
>
> So no, I think we need a better answer here.
>
>
> After thinking about this a lot, and trying and failing to implement less
> awful workarounds, I think AssertingVHes embedded in analysis results in
> fundamentally incompatible with caching of results.
>
> The cache gets invalidated automatically but not the instant the IR gets
> mutated. The assert happens too soon, and things blow up.
>
>
> Yeah, this is the crux of the problem and clearly incompatible with
> caching that is updated at the boundaries of transformation pass runs. The
> operations you're allowed to do or not on the IR should not depend on what
> analyses happen to be cached or not. For an analysis to hold an AssertingVH
> is basically saying "you cannot delete this part of the IR as long as I'm
> cached" which is not something an analysis should be allowed to do IMO.
>
> In principle, one alternative is to trigger the invalidation of the cached
> analysis result right before we delete the thing it is holding the
> AssertingVH on. But then in what sense in the AssertingVH actually
> "asserting"? At that point it is just a CallbackVH that triggers
> invalidation.
>
>
> I don't think we want to force cache invalidation logic in every pass that
> deletes a Value. =[
>
> So I think we should move away from AssertingVH in analysis results. If
> you need a more powerful debugging tool than ASan (or analogous) provides,
> we can build a DebugOnlyWeakVH or some such that becomes null immediately
> in debug builds. Or that has a asserting-only-if-used behavior rather than
> the eager assert we have today. But I'm inclined to build that tool when
> folks are first debugging something and tools like ASan are insufficient
> rather than eagerly.
>
>
> Any objections to this? I'd really like to nuke the 3 cases Sean
> identified in the tree (CallGraph, LVI, SCEV) and stop hacking around them.
>
>
> SGTM.
>
> -- Sean Silva
>
>
>
> -Chandler
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20170124/d1eff2e9/attachment.html>