[PATCH] D111836: [indvars] Use fact loop must exit to canonicalize to unsigned conditions

[Philip Reames via Phabricator via llvm-commits]

reames added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/IndVarSimplify.cpp:1840
+    // Some nested loops may share same folded exit basic block,
+    // thus we need to notify top most loop.
+    SE->forgetTopmostLoop(L);
----------------
mkazantsev wrote:
> I have a deja vu. :) We haven't folded any block here, right?
Unfortunately, I don't think so.  The problem is that this change could actually be changing the trip count.  If we'd otherwise figured out this exit was never taken (and thus the loop was full out UB), this converts the loop into a finite one, changing the trip count in the process.  This is legal (since the original cached value implied UB), but I don't think we can safely leave SCEV in a stale state.

We might be able to argue that we never cache the UB fact, but that makes me nervous since we've been generally teaching SCEV to exploit exactly that type of fact and even if we don't today, we might reasonable do so in the near future.  

While writing this, I did have an idea on how to do cheaper invalidation here, but it requires the use list mechanism you were proposing for SCEV, and lazily invalidating trip counts.  If you don't mind, I'll defer that to later.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111836/new/

https://reviews.llvm.org/D111836