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

[Liren.Peng via Phabricator via llvm-commits]

Peakulorain added inline comments.


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:7216
+  // FIXME: If loop has multi-exiting blocks, such as loop header and latch
+  // are both exiting block, max memory accesses time in loop could NOT
+  // represent loop max trip count. If we can get PostDominatorTree here,
----------------
reames wrote:
> Er, this is definitely untrue.  Or at least, if it is for your code, you're not computing a maximum at all and have a really nasty uncaught bug.
> 
> Any point where we execute UB which dominates the latch provides a term in the umin expression which defines the maximum number of times the latch can run.  
Fixed, please see my code comment


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:7260
+      // Or repeat memory opreation.
+      if (Step->getAPInt().getZExtValue() !=
+              ElemSize->getAPInt().getZExtValue() ||
----------------
reames wrote:
> Neither value is required to be less than 64 bits.
Fixed.


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:7278
+      const SCEV *MemSize = getConstant(Step->getType(), ObjSize);
+      auto *TripCount = dyn_cast<SCEVConstant>(getUDivExpr(MemSize, Step));
+      if (!TripCount)
----------------
reames wrote:
> You have multiple problems here:
> 1) Object size in is bytes, step is not.  I strongly recommend using bytes consistently in the code.  
> 2) An out of bounds access is not necessarily UB, see overall comment.  
Hmmm, I think step is in bytes here.


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:7281
+        continue;
+      unsigned UTripCount = (unsigned)TripCount->getAPInt().getZExtValue();
+      InferTripCount = std::min(InferTripCount, UTripCount);
----------------
reames wrote:
> Not guaranteed to be less than 64 bits.
fixed.


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

https://reviews.llvm.org/D109821