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

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

Ok, mix of nitpicks and three interesting bugs.  As a next step here, I think we should fix the nitpicks and leave the three interesting bug cases as follow-on patches.  This is not actually hooked into anything, and as such, landing the patch with known bugs won't break anything.  Several of the remaining bugs are of the variety that simply fixing them is easier than describing the fix.  :)

Given that, if you want to fix the couple of nitpicks, and repost, I'll LGTM.  I will require that the three issues are fixed before we enable this anywhere, but at least we can work incrementally.



================
Comment at: llvm/include/llvm/Analysis/ScalarEvolution.h:794
+  /// Returns the upper bound of the loop trip count infered from array size.
+  /// Data references in a loop should not access elements over the statically
+  /// allocated size.
----------------
rephrase as: "can not access bytes starting outside the statically allocated size without being immediate UB".

Two important details there:
* starting out of bounds
* not "should" - full undefined


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:7353
+      // Make sure only handle normal array.
+      auto *Ty = dyn_cast<ArrayType>(AllocateInst->getAllocatedType());
+      auto *ArrSize = dyn_cast<ConstantInt>(AllocateInst->getArraySize());
----------------
FYI, we can do better here by using getAllocationSizeInBits, but please *don't* change this in the current patch.  We're converging towards a LGTM, and minimal change is good.  It would make a good follow up though.


================
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));
----------------
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.  


================
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
----------------
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.


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:7382
+
+  return getUMinExpr(InferCountColl);
+}
----------------
You want getUMinExprFromMismatchedTypes here.


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

https://reviews.llvm.org/D109821