[PATCH] D51582: [IndVars] Turn isValidRewrite into an assertion

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev updated this revision to Diff 163963.
mkazantsev edited the summary of this revision.
mkazantsev added a comment.

Added clarification comment, updated commit message to describe what is going on.

In https://reviews.llvm.org/D51582#1223389, @atrick wrote:

> `isValidRewrite` is a hack that wasn't supposed to live this long.
>
> At the IR level, the problem is simply that we don't want to rewrite
>
> `gep Base, (&p[n] - &p[0])`
>
> as
>
> `gep &p[n], Base - &p[0]`
>
> SCEV no longer does the kind of reassociation. I'm not sure when it was fixed, but the original test case definitely no longer requires the `isValidRewrite` check.


Thanks for explanation! Is the "original test case" you are mentioning present somewhere in the unit tests? If no, I'd appreciate if you could check it in.

Do you guys feel OK if we check in this patch (leaving this as assertion) with the comment update I've just made?


https://reviews.llvm.org/D51582

Files:
  lib/Transforms/Scalar/IndVarSimplify.cpp


Index: lib/Transforms/Scalar/IndVarSimplify.cpp
===================================================================
--- lib/Transforms/Scalar/IndVarSimplify.cpp
+++ lib/Transforms/Scalar/IndVarSimplify.cpp
@@ -136,7 +136,9 @@
   SmallVector<WeakTrackingVH, 16> DeadInsts;
   bool Changed = false;
 
+#ifndef NDEBUG
   bool isValidRewrite(Value *FromVal, Value *ToVal);
+#endif
 
   void handleFloatingPointIV(Loop *L, PHINode *PH);
   void rewriteNonIntegerIVs(Loop *L);
@@ -163,18 +165,17 @@
 
 } // end anonymous namespace
 
+#ifndef NDEBUG
 /// Return true if the SCEV expansion generated by the rewriter can replace the
 /// original value. SCEV guarantees that it produces the same value, but the way
-/// it is produced may be illegal IR.  Ideally, this function will only be
-/// called for verification.
+/// it is produced may be illegal IR.
 bool IndVarSimplify::isValidRewrite(Value *FromVal, Value *ToVal) {
   // If an SCEV expression subsumed multiple pointers, its expansion could
   // reassociate the GEP changing the base pointer. This is illegal because the
   // final address produced by a GEP chain must be inbounds relative to its
   // underlying object. Otherwise basic alias analysis, among other things,
-  // could fail in a dangerous way. Ultimately, SCEV will be improved to avoid
-  // producing an expression involving multiple pointers. Until then, we must
-  // bail out here.
+  // could fail in a dangerous way. Specifically, we want to make sure that SCEV
+  // does not convert gep Base, (&p[n] - &p[0]) into gep &p[n], Base - &p[0].
   //
   // Retrieve the pointer operand of the GEP. Don't use GetUnderlyingObject
   // because it understands lcssa phis while SCEV does not.
@@ -214,6 +215,7 @@
   }
   return true;
 }
+#endif
 
 /// Determine the insertion point for this user. By default, insert immediately
 /// before the user. SCEVExpander or LICM will hoist loop invariants out of the
@@ -640,10 +642,7 @@
                           << '\n'
                           << "  LoopVal = " << *Inst << "\n");
 
-        if (!isValidRewrite(Inst, ExitVal)) {
-          DeadInsts.push_back(ExitVal);
-          continue;
-        }
+        assert(isValidRewrite(Inst, ExitVal) && "Must be!");
 
 #ifndef NDEBUG
         // If we reuse an instruction from a loop which is neither L nor one of


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D51582.163963.patch
Type: text/x-patch
Size: 2335 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180905/32aa4045/attachment.bin>