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

[Philip Reames via Phabricator via llvm-commits]

reames requested changes to this revision.
reames added a comment.
This revision now requires changes to proceed.

This change as posted has a number of issues, some minor, some major.

The core issue I see with this change is that it's making assumptions about out of bounds accesses being immediate UB which are not necessarily true.  Skimming the LangRef for the load instruction, I find nothing that requires an out of bounds load to be immediate UB.  (Returning poison could be legal.)  And partially out of bounds accesses are specifically mentioned as legal in the alignment discussion.  At a minimum, that means there's an off by one in the max term.  Before we can move forward with this change, we need to discuss the desired semantics and clarify the langref.  Keep in mind that there's a major gap between "we can prove this safe to access" and "we can prove this will fault on access".

On the more minor side, a couple of issues worth noting.

First, the easy stuff.  The code mixes units resulting in incorrect results.  The code doesn't account for values larger than 64 bits.

Second, I recommend we go back to something closer to the original patch and work incrementally.  Specifically, I recommend that we do *not* make the suggested getObjectSize change and integration into general SCEV trip count logic.  Both of those can come later.  I see strong value in minimalism to a) allow easier review and b) minimize fallout.  Once checked in and enabled, doing each of those enhancement seems well justified.



================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:7200
+  unsigned MaxTripCount = getConstantTripCount(MaxExitCount);
+  unsigned InferedTripCount = getConstantMaxTripCountFromArray(L);
+  if (MaxTripCount != 0 && InferedTripCount != 0) {
----------------
This is the wrong place to integrate this.  See overall comment.


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


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


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


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


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

https://reviews.llvm.org/D109821