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

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev accepted this revision.
mkazantsev added a comment.
This revision is now accepted and ready to land.

LGTM, but I still think that "folded" is not a proper term here.



================
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);
----------------
reames wrote:
> 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.
I don't mind that you forget the loop. My comment was about statement that we've folded something, when we didn't. Maybe I just misunderstand what "folded" meant, but it was about changing some condition to true/false, which is not the case here.


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

https://reviews.llvm.org/D111836