[PATCH] D155049: [ScalarEvolution] Infer loop max trip count from memory accesses

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

I do not follow parts of this. The overlapping stuff is odd. We should also filter access earlier. And finally, lot's of the checks seem weird.
What I was expecting is roughly:

Loop count C is unknown.
Size of the object is T.
Access size is A.
Access function is F, we can restrict it to <B, +, S>loop with S > 0.

What we know is that 
(T - B) >= 0
or the loop trip count is 0.
And
T >= C * (S * A) + B
which is
(T - B) >= C * S * A
which gives us, under some checks
(T - B) / (S * A) >= C
The left hand side should be constant and thereby bound C.

What am I missing?



================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:255
+    cl::desc("Infer loop max trip count from memory access"),
+    cl::init(false));
+
----------------
Do we have compile time numbers with this set to true?


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:8184-8195
+  // TODO: Memory which has certain size.
+  AllocaInst *AllocateInst = dyn_cast<AllocaInst>(PtrBase->getValue());
+  if (!AllocateInst)
+    return nullptr;
+
+  // Make sure only handle normal array.
+  auto *Ty = dyn_cast<ArrayType>(AllocateInst->getAllocatedType());
----------------
We have a helper for this `llvm::getObjectSize`

https://llvm.org/doxygen/namespacellvm.html#a62ba0e5ee2d86f663c6de4efda6082a7



================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:8252-8255
+    if (!memReadWriteMaybeOverlapping(I, AddRec, this)) {
+      Exprs.insert(AddRec);
+      IdxWrapMap[AddRec] = cast<SCEVConstant>(IdxWrap);
+    }
----------------
I also don't understand why we need to check overlapping. 


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:8263-8266
+    const SCEV *MemSize =
+        getCertainSizeOfMem(PtrBase, Step->getType(), DL, this);
+    if (!MemSize)
+      continue;
----------------
I would have expected this code much earlier. The expectation is that most pointer bases are not bounded, so why do we bother collecting the instructions, checking the access expression, etc. We should look at the base first before we ever consider using the access for reasoning.


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:8272-8281
+
+    // If the loop reaches the maximum number of executions, we can not
+    // access bytes starting outside the statically allocated size without
+    // being immediate UB. But it is allowed to enter loop header one more
+    // time.
+    auto *InferCount = dyn_cast<SCEVConstant>(
+        getAddExpr(MaxExeCount, getOne(MaxExeCount->getType())));
----------------
I doubt this is necessary given the other restrictions. You already verified the access happens every iteration and it will happen whenever we enter the loop header. This is only necessary if we relax the conditions and allow early exists. Thus, add 1 only if the existing block is not unique or not the latch.


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:8291
+    if (InferVC->getValue().getZExtValue() > WrapVC->getValue().getZExtValue())
+      continue;
+
----------------
I don't understand this.


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:8321
       dyn_cast<SCEVConstant>(getConstantMaxBackedgeTakenCount(L));
-  return getConstantTripCount(MaxExitCount);
+  unsigned MaxExitCountN = getConstantTripCount(MaxExitCount);
+  if (UseMemoryAccessUBForBEInference) {
----------------
If we got a proper value, no need to use the secondary reasoning, right?


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

https://reviews.llvm.org/D155049