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

[Hal Finkel via llvm-dev]

On 01/23/2017 03:08 AM, Chandler Carruth 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 
> <mailto: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.

I agree; having an analysis that all clients need to promise not to 
invalidate in certain ways is troublesome.

  -Hal

> 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.111
>
>
> 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.
>
> -Chandler

-- 
Hal Finkel
Lead, Compiler Technology and Programming Languages
Leadership Computing Facility
Argonne National Laboratory

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20170123/797abea6/attachment.html>