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

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev 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);
+
----------------
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>`?


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:7287
+      // Only handle { %array + step },
+      // FIXME: {(SCEVAddRecExpr) + step } could not be analysised here.
+      if (AddRec->getStart() != ArrBase)
----------------
typo: analysised  -> analysed


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:7307
+      // Make sure only handle normal array.
+      Type *Ty = AllocateInst->getAllocatedType();
+      auto *ArrSize = dyn_cast<ConstantInt>(AllocateInst->getArraySize());
----------------
dyn_cast here to avoid isa and anyther cast below.


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:7315
+
+      // Now we can infer a max excute time by MemLength/StepLength.
+      const SCEV *MemSize =
----------------
Is max execute time a same thing as trip count, or it's something else? If they are the same, please use "trip count" as something that is more understandable.


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:7192
+  // We can't infer from Array in Irregular Loop.
+  // FIXME: It's hard to infer if loop has multi-exiting blocks.
+  // FIXME: It's hard to infer loop bound from array operated in Nested Loop.
----------------
Peakulorain wrote:
> nikic wrote:
> > Why are multiple exits a problem? This is calculating an upper bound, so even if we exit before that should still be a valid max trip count. (If multiple exits were a problem, you would also have to guard against abnormal exits.)
> Thank you very much for your comments. 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, MemAccessBB post dom loop header can make sure that valid trip count. So we can improve this function after PostDom info could be got.
But isn't max trip count an upper extimate of the actual trip count? I mean, no abnormal exit may *increase* the number of iterations made in loop. It can only exit earlier than what you've computed otherwise.


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

https://reviews.llvm.org/D109821