[PATCH] D29395: [ValueTracking] avoid crashing from bad assumptions (PR31809)

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: llvm/trunk/lib/Analysis/ValueTracking.cpp:800-803
+  // FIXME: Implement a stronger version of "I give up" by invalidating/clearing
+  // the assumption cache. This should indicate that the cache is corrupted so
+  // future callers will not waste time repopulating it with faulty assumptions.
+
----------------
hfinkel wrote:
> davide wrote:
> > davide wrote:
> > > Have you looked into how hard it would be to implement this invalidation? Also, why is that a stronger version? (i.e. there are cases where this actually matters or you're just speculating?)
> > > Also, clearing the whole assumption cache is really what we want? I mean, what if there are other informations that are not inconsistent and we want to use them anyway?
> > In other words, I'm not entirely sure we should add this `FIXME`.
> I agree. This is a rare case and I don't think we need to make the infrastructure more complicated than necessary to handle it. Plus, it is a useful invariant that all of the assumptions are in the cache.
Yes, I looked at putting Q.AC->clear() here. Let me attach a draft of that patch (or should I open a new review?).
I think that would be stronger in the sense that we'd be less likely to "leak" a transform based on a bad assumption. 

I have no evidence that this matters or works the way I'm imagining (other than it affects the test cases included in this patch). That's why I didn't include it here. I was (possibly incorrectly) assuming that once we hit a bad assumption, we go into "all bets are off" mode, so we might as well nuke the cache. Let's remove the FIXME if that's wrong.

Personally, I think emitting the warning is the more important thing to do because we're now silently continuing compilation when we know something bad has happened.


Repository:
  rL LLVM

https://reviews.llvm.org/D29395