[PATCH] D42365: [LoopFlatten] Add a loop-flattening pass

[Sam Parker via Phabricator via llvm-commits]

samparker added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/LoopFlatten.cpp:141
+  }
+  if (!Increment || Increment->hasNUsesOrMore(3)) {
+    LLVM_DEBUG(dbgs() << "Cound not find valid increment\n");
----------------
How are the limits decided for the number of users? My counting seems to be off-by-one for both the compare and the increment.


================
Comment at: llvm/lib/Transforms/Scalar/LoopFlatten.cpp:150
+  assert(InductionPHI->getNumIncomingValues() == 2);
+  if (InductionPHI->getIncomingValueForBlock(Latch) != Increment) {
+    LLVM_DEBUG(dbgs() << "PHI value is not increment inst\n");
----------------
Doesn't the pattern matching make this check redundant?


================
Comment at: llvm/lib/Transforms/Scalar/LoopFlatten.cpp:213
+    // The other incoming value must come from the inner loop, without any
+    // modifications in the tail end of the outer loop. We are in LCSSA form,
+    // so this will actually be a PHI in the inner loop's exit block, which
----------------
Do we check for LCSSA? Or is this covered by it being a 'simple' loop?


================
Comment at: llvm/lib/Transforms/Scalar/LoopFlatten.cpp:260
+  for (auto *B : OuterLoop->getBlocks()) {
+    if (!InnerLoop->contains(B)) {
+      for (auto &I : *B) {
----------------
reduce nesting here?


================
Comment at: llvm/lib/Transforms/Scalar/LoopFlatten.cpp:390
+        // would be UB.
+        if (GEP->isInBounds() &&
+            V->getType()->getIntegerBitWidth() >=
----------------
Is inbounds not enough? Also, why do we return on the first valid gep, don't we have to check all the users?


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

https://reviews.llvm.org/D42365