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

Sean Silva via llvm-dev llvm-dev at lists.llvm.org
Mon Jan 23 22:07:27 PST 2017


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/20170123/35017c81/attachment.html>


More information about the llvm-dev mailing list