[PATCH] D53027: [LoopInterchange] Fix inner loop reduction handling.

[Eli Friedman via Phabricator via llvm-commits]

efriedma added a comment.

This analysis seems very fragile.



================
Comment at: lib/Transforms/Scalar/LoopInterchange.cpp:696
+      int IncInd = PHI.getBasicBlockIndex(L->getLoopLatch());
+      if (PHInd == -1 || IncInd == -1)
+        return false;
----------------
I'm pretty sure the "PHInd == -1 || IncInd == -1" check is unnecessary, given the PHI is in the loop header.


================
Comment at: lib/Transforms/Scalar/LoopInterchange.cpp:712
+
+            if ((SI = dyn_cast<StoreInst>(&*U2)) &&
+                SI->getParent() == L->getExitBlock())
----------------
What happens if SI is non-null before we enter here?


================
Comment at: lib/Transforms/Scalar/LoopInterchange.cpp:715
+              continue;
+            SI = nullptr;
+          }
----------------
What happens if there are mutiple stores using the PHI?


================
Comment at: lib/Transforms/Scalar/LoopInterchange.cpp:722
+      if (!LI || !SI || SI->getOperand(1) != LI->getOperand(0))
+        return false;
+
----------------
Could any operations between the load and store alias them?  Does it matter?


https://reviews.llvm.org/D53027