[PATCH] D27216: [SCEVExpand] do not hoist divisions by zero (PR30935)

[Sanjoy Das via Phabricator via llvm-commits]

sanjoy added a comment.

Hi,

I don't think this patch properly addresses PR30976 -- it only hides the symptom.  For instance, this new test case crashes SCEVExpander even with this fix: https://reviews.llvm.org/differential/diff/79708/

I think the fundamental problem is that there is no clear distinction in SCEVExpander between hoisting for optimization and hoisting for correctness (i.e. for the right domination properties to hold).  We need to fix that before we can fix PR30976.



================
Comment at: llvm/lib/Analysis/ScalarEvolutionExpander.cpp:170
+// Return true when S may contain the value zero.
+static inline bool mayBeValueZero(Value *V) {
+  if (ConstantInt *C = dyn_cast<ConstantInt>(V))
----------------
Why do you need the `inline` keyword here?



================
Comment at: llvm/lib/Analysis/ScalarEvolutionExpander.cpp:1679
   Instruction *InsertPt = &*Builder.GetInsertPoint();
-  for (Loop *L = SE.LI.getLoopFor(Builder.GetInsertBlock());;
-       L = L->getParentLoop())
-    if (SE.isLoopInvariant(S, L)) {
-      if (!L) break;
-      if (BasicBlock *Preheader = L->getLoopPreheader())
-        InsertPt = Preheader->getTerminator();
-      else {
-        // LSR sets the insertion point for AddRec start/step values to the
-        // block start to simplify value reuse, even though it's an invalid
-        // position. SCEVExpander must correct for this in all cases.
-        InsertPt = &*L->getHeader()->getFirstInsertionPt();
-      }
-    } else {
-      // If the SCEV is computable at this level, insert it into the header
-      // after the PHIs (and after any other instructions that we've inserted
-      // there) so that it is guaranteed to dominate any user inside the loop.
-      if (L && SE.hasComputableLoopEvolution(S, L) && !PostIncLoops.count(L))
-        InsertPt = &*L->getHeader()->getFirstInsertionPt();
-      while (InsertPt->getIterator() != Builder.GetInsertPoint() &&
-             (isInsertedInstruction(InsertPt) ||
-              isa<DbgInfoIntrinsic>(InsertPt))) {
-        InsertPt = &*std::next(InsertPt->getIterator());
+  if (!SCEVExprContains(S, [](const SCEV *S) {
+        if (const auto *D = dyn_cast<SCEVUDivExpr>(S)) {
----------------
This looks too complicated to be within the condition of an `if` -- can you extract out a `bool SafeToHoist = ...`?


================
Comment at: llvm/lib/Analysis/ScalarEvolutionExpander.cpp:1687
+          // All other divisions should not be moved as they may be divisions by
+          // zero and should be kept within the conditions of the surrounding
+          // loops that guard their execution (see PR30935.)
----------------
Why not `return SC->getValue()->isZero();`?


https://reviews.llvm.org/D27216