[PATCH] D90640: [LoopFlatten] Widen the IV

[Dave Green via Phabricator via llvm-commits]

dmgreen added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/LoopFlatten.cpp:335
+  Value *InnerLimit = FI.InnerLimit;
+  if (auto *I  = dyn_cast<SExtInst>(InnerLimit))
+    InnerLimit = I->getOperand(0);
----------------
Some formatting is off here.


================
Comment at: llvm/lib/Transforms/Scalar/LoopFlatten.cpp:348
+    // look through truncs.
+    if (dyn_cast<TruncInst>(U))
+      U = *U->user_begin();
----------------
I think this should be checking each use of the trunk, or checking the trunk has one use.


================
Comment at: llvm/lib/Transforms/Scalar/LoopFlatten.cpp:394
+
+    if (User *V = dyn_cast<TruncInst>(U)) {
+      for (auto *K : V->users()) {
----------------
auto *V = 


================
Comment at: llvm/lib/Transforms/Scalar/LoopFlatten.cpp:597
+  if (InnerType != OuterType ||
+      InnerType->getScalarSizeInBits() == MaxLegalSize) {
+    LLVM_DEBUG(dbgs() << "Can't widen the IV\n");
----------------
 >= ?

I feel like something here needs to be checking that newbitwidth > InnerType->getScalarSizeInBits() * 2
(Which likely won't be very important for power-2 type sizes, but is still needed to make sure the new IV doesn't overflow?)


================
Comment at: llvm/lib/Transforms/Scalar/LoopFlatten.cpp:610
+
+  AddCandidatePhi(FI.InnerLoop->getHeader()->begin());
+  AddCandidatePhi(FI.OuterLoop->getHeader()->begin());
----------------
Do we need to be widening all the phi's, or just the induction?


================
Comment at: llvm/lib/Transforms/Scalar/LoopFlatten.cpp:617
+  SCEVExpander Rewriter(*SE, DL, "loopflatten");
+  SmallVector<WeakTrackingVH, 4> DeadInsts;
+
----------------
What does DeadInstr contain after the calls to WidenIV? Should they be removed now, does that simplify things later on?


================
Comment at: llvm/lib/Transforms/Scalar/LoopFlatten.cpp:653
+    // After widening, rediscover all the loop components.
+    if (!CanFlattenLoopPair(FI, DT, LI, SE, AC, TTI, markLoopAsDeleted))
+      return false;
----------------
Can this call findLoopComponents directly, or is there something in CanFlattenloopPair that would be needed?


================
Comment at: llvm/test/Transforms/LoopFlatten/widen-iv.ll:90
+
+for.cond1.preheader:                              ; preds = %for.cond1.preheader.preheader, %for.cond1.preheader
+  %i.018 = phi i32 [ %inc6, %for.cond1.preheader ], [ 0, %for.cond1.preheader.preheader ]
----------------
Can this test be cleaned up a bit (or perhaps a simpler test be added). This seems to be loop unswitched with this extra remainder loop left over.


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

https://reviews.llvm.org/D90640