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

[Dave Green via Phabricator via llvm-commits]

dmgreen marked 5 inline comments as done.
dmgreen added a comment.

Sorry for the delay here. I asked for review when I got some suddenly realized it had been years since I actually looked at this (and didn't write it to begin with!)  And I apparently didn't have the time right now to look into the details again.

On a conceptual level, it would also be good if it wasn't just me and "the person sat opposite me" (current situation withstanding) that looked at this. I believe large(ish) target independent changes like this should preferably get looked at by more than just one group.

If there is anyone else out there that is interested in it, please feel free to take a look or take it off my hands :)



================
Comment at: llvm/lib/Transforms/Scalar/LoopFlatten.cpp:141
+  }
+  if (!Increment || Increment->hasNUsesOrMore(3)) {
+    LLVM_DEBUG(dbgs() << "Cound not find valid increment\n");
----------------
samparker wrote:
> 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.
Increment will be used by Phi and Cmp, Cmp will be used by Br I think.


================
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");
----------------
samparker wrote:
> Doesn't the pattern matching make this check redundant?
You are likely correct, but safer to keep it around?


================
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
----------------
samparker wrote:
> Do we check for LCSSA? Or is this covered by it being a 'simple' loop?
This being a LoopPass implies LCSSA form.


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


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


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

https://reviews.llvm.org/D42365