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

> On 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 <mailto: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
<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.
> 
> 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.
Another view of it is that there should be another handle that triggers the
invalidation of the analysis when this IR is changed: i.e. keeping the analysis
cached while it holds a reference to the IR can be seen as the problem.

— 
Mehdi


> 
> 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/4f68f35f/attachment-0001.html>

On Tue, Jan 24, 2017 at 8:19 AM Mehdi Amini via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
>
> Another view of it is that there should be another handle that triggers
> the invalidation of the analysis when this IR is changed: i.e. keeping the
> analysis cached while it holds a reference to the IR can be seen as the
> problem.
>
The issue is only with Asserting VH, not with other kinds of value handles
though. So with those, we would have different invalidation in NDEBUG from
!NDEBUG? Or would you always do this invalidation?

I guess I'm not too clear what the concrete approach here would be.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20170124/54e9ea1b/attachment.html>

> On Jan 24, 2017, at 11:59 AM, Chandler Carruth <chandlerc at
gmail.com> wrote:
> 
> On Tue, Jan 24, 2017 at 8:19 AM Mehdi Amini via llvm-dev <llvm-dev at
lists.llvm.org <mailto:llvm-dev at lists.llvm.org>> wrote:
> 
> Another view of it is that there should be another handle that triggers the
invalidation of the analysis when this IR is changed: i.e. keeping the analysis
cached while it holds a reference to the IR can be seen as the problem.
> 
> The issue is only with Asserting VH, not with other kinds of value handles
though. So with those, we would have different invalidation in NDEBUG from
!NDEBUG? Or would you always do this invalidation?
Not sure why would the invalidation be different? If you have an AssertingVH in
your analysis, you need a mechanism to invalidate it before the assertion fires.
But that invalidation needs to happens both in NDEBUG and !NDEBUG consistently
(even if there wouldn’t be an assertion in NDEBUG mode if this is violated) .
> I guess I'm not too clear what the concrete approach here would be.
I should have mentioned that I didn’t give much thought about a concrete plan,
it was more an abstract view on the semantic of holding AssertingVH.
Deciding that AssertingVH is the wrong semantic for a given analysis can be OK
(I believe this is what you did right?), and there might not be any practical
alternative (to provide invalidation on the fly when IR is deleted).

— 
Mehdi

-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20170124/9be1c04a/attachment.html>