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

[Chandler Carruth via llvm-dev]

On Mon, Jan 23, 2017 at 1:09 AM Chandler Carruth via llvm-dev <
llvm-dev at lists.llvm.org> 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. 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.
>

FYI: the initial patch doing exactly this and cleaning up the new PM as a
consequence is here: https://reviews.llvm.org/D29006

Just figured I'd give folks a concrete idea of the impact and scope of this.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20170123/13fd9f95/attachment.html>