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

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev created this revision.
mkazantsev added reviewers: atrick, wmi, sanjoy, reames.
Herald added a subscriber: javed.absar.

Function rewriteLoopExitValues contains a check on isValidRewrite which
describes some strange situation about SCEV messing up with GEPs, but no
test actually demonstrates this behavior. I wasn't able to find one by running
more than 5000 fuzz tests as well. The comment in this method doesn't give
any sound description of how and what may go wrong.

It was introduced by https://reviews.llvm.org/rL127839 without any associated test. The issue this patch
claims to fix (rdar://problem/9038671) cannot be found by now. The patch is pretty
big and it's unclear whether or not this particular check has anything to do with the
problem it was supposed to address.

There are two possible options:

1. This piece of code is obsolete by now. We should leave it as assert or just remove it later.
2. This piece of code really protects us from some nasty bug for which we don't have a unit test. In this case the check should be returned and the respective unit test should be added.

This patch turns this piece of code into an assertion to diagnose what of two options
above is true. We will either improve compile time or improve test coverage by checking
it in.


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);
@@ -166,10 +168,10 @@
 
 } // 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
@@ -217,6 +219,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
@@ -656,10 +659,7 @@
                           << '\n'
                           << "  LoopVal = " << *Inst << "\n");
 
-        if (!isValidRewrite(Inst, ExitVal)) {
-          DeadInsts.push_back(ExitVal);
-          continue;
-        }
+        assert(isValidRewrite(Inst, ExitVal) && "Must be!");
 
         // Collect all the candidate PHINodes to be rewritten.
         RewritePhiSet.emplace_back(PN, i, ExitVal, HighCost);


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D51582.163664.patch
Type: text/x-patch
Size: 1726 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180903/e56eb232/attachment.bin>