[PATCH] D109821: [ScalarEvolution] Infer loop max trip count from array accesses

[Philip Reames via Phabricator via llvm-commits]

reames accepted this revision.
reames added a comment.

LGTM w/one required change before landing.

For anyone following along, note that this patch is being committed off-by-default with a known bug (noted in review) to make it easier to iterate and collaborate.  This is not yet ready for general use.



================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:7365
+      const SCEV *MemSize =
+          getConstant(Step->getType(), DL.getTypeAllocSize(Ty));
+      auto *MaxExeCount = dyn_cast<SCEVConstant>(getUDivExpr(MemSize, Step));
----------------
Peakulorain wrote:
> reames wrote:
> > Ok, there's a subtle bug here.
> > 
> > Since gep indices are silently zext to the indexing type, you can have a narrow gep index which wraps around rather than strictly increasing.  You can fix this by ensuring that the addrec you checked for above has the same type as the gep.  
> Can we use "hasNoSelfWrap()" to ensure that? I used this API above.
No self wrap is not enough.  You could have a index size chosen to just barely unsigned wrap, but not self-wrap in the addressed space.

Please drop this line, and we can revisit in a follow up.  


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:7370
+
+      // If the loop reaches the maximum number of executions, and UB will
+      // definitely occur in the next iteration, it is allowed to enter loop
----------------
Peakulorain wrote:
> reames wrote:
> > Ok, you have two different bugs here.
> > 
> > First, a *partially* out of bounds access is allowed without being UB.  (Or at least, that seemed to be the conclusion we'd come to earlier.)  Since the above udiv produces a floor (not a ceiling), you have the possibility of one extra iteration being well defined with an access that starts in bounds and finishes out of bounds.  Easiest fix would be to the use getUDivCeilSCEV in the line above.
> > 
> > Second, in the case where element size is 1 (i.e. a byte array), you're udiv above can produce uint_max for the respective type.  As such, the increment below can wrap, and you need to use a wider type.
> > Ok, you have two different bugs here.
> > 
> > First, a *partially* out of bounds access is allowed without being UB.  (Or at least, that seemed to be the conclusion we'd come to earlier.)  Since the above udiv produces a floor (not a ceiling), you have the possibility of one extra iteration being well defined with an access that starts in bounds and finishes out of bounds.  Easiest fix would be to the use getUDivCeilSCEV in the line above.
> > 
> Thanks for your remind, fixed.
> 
> > Second, in the case where element size is 1 (i.e. a byte array), you're udiv above can produce uint_max for the respective type.  As such, the increment below can wrap, and you need to use a wider type.
> We discard such an extremum value.
> 
I don't see how the second case is prevented in code.,  But that's fine, we'll revisit once landed.


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

https://reviews.llvm.org/D109821