<div dir="ltr">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.<div><br></div><div>Thanks!</div><div>-Chandler</div></div><br><div class="gmail_quote"><div dir="ltr">On Mon, Jan 23, 2017 at 10:07 PM Sean Silva <<a href="mailto:chisophugis@gmail.com">chisophugis@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr" class="gmail_msg"><div class="gmail_extra gmail_msg"><div class="gmail_quote gmail_msg">On Mon, Jan 23, 2017 at 1:08 AM, Chandler Carruth <span dir="ltr" class="gmail_msg"><<a href="mailto:chandlerc@gmail.com" class="gmail_msg" target="_blank">chandlerc@gmail.com</a>></span> wrote:<br class="gmail_msg"><blockquote class="gmail_quote gmail_msg" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr" class="gmail_msg">This thread kinda died. I'd like to revive it.<div class="gmail_msg"><br class="gmail_msg"></div><div class="gmail_msg">The new PM stuff is making excellent progress, and this is actually one of the last things to clean up.</div><div class="gmail_msg"><br class="gmail_msg"><div class="gmail_quote gmail_msg"><span class="gmail_msg"><div dir="ltr" class="gmail_msg">On Mon, Aug 8, 2016 at 1:10 AM Sean Silva <<a href="mailto:chisophugis@gmail.com" class="gmail_msg" target="_blank">chisophugis@gmail.com</a>> wrote:<br class="gmail_msg"></div><blockquote class="gmail_quote gmail_msg" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr" class="m_5006366296995659484m_1685334894397508985gmail_msg gmail_msg"><div class="gmail_extra m_5006366296995659484m_1685334894397508985gmail_msg gmail_msg"><div class="gmail_quote m_5006366296995659484m_1685334894397508985gmail_msg gmail_msg"><blockquote class="gmail_quote m_5006366296995659484m_1685334894397508985gmail_msg gmail_msg" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr" class="m_5006366296995659484m_1685334894397508985gmail_msg gmail_msg"><div class="m_5006366296995659484m_1685334894397508985gmail_msg gmail_msg">Thoughts? For the moment I have put in a workaround (r274457) that makes jump-threading invalidate LVI.</div></div></blockquote><div class="m_5006366296995659484m_1685334894397508985gmail_msg gmail_msg"><br class="m_5006366296995659484m_1685334894397508985gmail_msg gmail_msg"></div></div></div></div><div dir="ltr" class="m_5006366296995659484m_1685334894397508985gmail_msg gmail_msg"><div class="gmail_extra m_5006366296995659484m_1685334894397508985gmail_msg gmail_msg"><div class="gmail_quote m_5006366296995659484m_1685334894397508985gmail_msg gmail_msg"><div class="m_5006366296995659484m_1685334894397508985gmail_msg gmail_msg">Is everybody happy with this workaround?</div></div></div></div></blockquote><div class="gmail_msg"><br class="gmail_msg"></div></span><div class="gmail_msg">I wasn't too happy with it, but I had no better suggestion.</div><div class="gmail_msg"><br class="gmail_msg"></div><div class="gmail_msg">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. =[</div><div class="gmail_msg"><br class="gmail_msg"></div><div class="gmail_msg">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:</div><div class="gmail_msg"><br class="gmail_msg"></div><div class="gmail_msg">#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 ::</div><div class="gmail_msg"><br class="gmail_msg"></div><div class="gmail_msg">#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.</div><div class="gmail_msg"><br class="gmail_msg"></div><div class="gmail_msg">So no, I think we need a better answer here.</div><div class="gmail_msg"><br class="gmail_msg"></div><div class="gmail_msg"><br class="gmail_msg"></div><div class="gmail_msg">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.</div><div class="gmail_msg"><br class="gmail_msg"></div><div class="gmail_msg">The cache gets invalidated automatically but not the instant the IR gets mutated. The assert happens too soon, and things blow up.</div></div></div></div></blockquote><div class="gmail_msg"><br class="gmail_msg"></div></div></div></div><div dir="ltr" class="gmail_msg"><div class="gmail_extra gmail_msg"><div class="gmail_quote gmail_msg"><div class="gmail_msg">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.</div><div class="gmail_msg"><br class="gmail_msg"></div><div class="gmail_msg">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.</div></div></div></div><div dir="ltr" class="gmail_msg"><div class="gmail_extra gmail_msg"><div class="gmail_quote gmail_msg"><div class="gmail_msg"> </div><blockquote class="gmail_quote gmail_msg" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr" class="gmail_msg"><div class="gmail_msg"><div class="gmail_quote gmail_msg"><div class="gmail_msg"> I don't think we want to force cache invalidation logic in every pass that deletes a Value. =[</div><div class="gmail_msg"><br class="gmail_msg"></div><div class="gmail_msg">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.</div><div class="gmail_msg"><br class="gmail_msg"></div><div class="gmail_msg"><br class="gmail_msg"></div><div class="gmail_msg">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.</div></div></div></div></blockquote><div class="gmail_msg"><br class="gmail_msg"></div></div></div></div><div dir="ltr" class="gmail_msg"><div class="gmail_extra gmail_msg"><div class="gmail_quote gmail_msg"><div class="gmail_msg">SGTM.</div><div class="gmail_msg"><br class="gmail_msg"></div><div class="gmail_msg">-- Sean Silva</div><div class="gmail_msg"> </div><blockquote class="gmail_quote gmail_msg" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr" class="gmail_msg"><div class="gmail_msg"><div class="gmail_quote gmail_msg"><span class="m_5006366296995659484HOEnZb gmail_msg"><font color="#888888" class="gmail_msg"><div class="gmail_msg"><br class="gmail_msg"></div><div class="gmail_msg">-Chandler</div></font></span></div></div></div>
</blockquote></div><br class="gmail_msg"></div></div>
</blockquote></div>