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

[Bin Cheng via Phabricator via llvm-commits]

bin.cheng added inline comments.


================
Comment at: llvm/include/llvm/Analysis/ScalarEvolution.h:794
+  /// Returns 0 if the trip count is unknown.
+  unsigned getConstantMaxTripCountFromArray(const Loop *L);
+
----------------
reames wrote:
> bin.cheng wrote:
> > mkazantsev wrote:
> > > I think it's a bad API for this. The methods above has an excuse of using word "small", assuming that they don't care about "big" trip counts (for whatever reason that is).
> > > 
> > > Your method sounds like it just computes an upper bound of trip count from array, whatever this value is. What if this value does not fit in unsigned? Any reasons we can't return an `Optional<APInt>`?
> > Right, it's possible the computed trip count can't be expressed by unsigned type.  Either we should return Optional<APInt> or rename it as getSmallConstantMaxTripCountFromArray and only returns non-zero if trip count can be expressed by unsigned type.
> > I can see two advantages with the latter choice:
> > 1\ be consistent with existing interfaces.
> > 2\ the interface is mostly designed to be used to avoid(or enable) various loop transformations in compilation time unknown small trip count. (like vectorization, loop idiom, more accurate BPI, etc..)
> > 
> > Do you have any preference on this?
> > 
> > Thanks
> I'd suggest switching this interface to simply return a SCEV - specifically a getUMinOfMismatchTypes.  
> 
> If we later want a version which only returns small constants (per the current definition), we can.
> 
> Also, please be *very* clear in the description of whether this is returning a backedge taken count (e.g. exit count) or a trip count.  The existing small constant interfaces are generally trip counts.  (In particular, zero has *very* different meanings for the two types of counts.)
> I'd suggest switching this interface to simply return a SCEV - specifically a getUMinOfMismatchTypes.  
> 
> If we later want a version which only returns small constants (per the current definition), we can.
> 
> Also, please be *very* clear in the description of whether this is returning a backedge taken count (e.g. exit count) or a trip count.  The existing small constant interfaces are generally trip counts.  (In particular, zero has *very* different meanings for the two types of counts.)

Thanks for reviewing.  I am not very sure about this one because the comment before this API already mentioned "Returns the upper bound of the loop trip count infered from array size.", so, the interface itself is all about trip count, not backedge taken count.  

Do you mean it's unclear because backedge taken count is used in computation of trip count upper bound?  If so, we can simplify this patch by only handle "latch_block == exiting_block" case just as you suggested.  

Is this something what you want to have?  please point out if I've missed something important.

Thanks very much again.


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

https://reviews.llvm.org/D109821