[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:
> 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.
Oh!  That's completely not what I thought you meant.

I actually agree.  The phrasing "folded exit block" is odd.  However, it's verbatim the comment used elsewhere in the file for the same case, and I figured consistency was better than perfection.  

It's also worth noting this is majorly conservative if the outer loop *doesn't* actually share an exit block.  We really should be walking out only a far as actually needed.  But since we've got one major rewrite of SCEV invalidation in flight, I didn't want to start another at the same time.  


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111836