[PATCH] D90640: [LoopFlatten] Widen the IV

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer added inline comments.


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

I didn't see how the inner type can be larger than the legal max size, but since `>` certainly doesn't hurt I will add that.


================
Comment at: llvm/lib/Transforms/Scalar/LoopFlatten.cpp:610
+
+  AddCandidatePhi(FI.InnerLoop->getHeader()->begin());
+  AddCandidatePhi(FI.OuterLoop->getHeader()->begin());
----------------
dmgreen wrote:
> Do we need to be widening all the phi's, or just the induction?
I borrowed this logic that drives createWideIV from IndVarSimplify, but that might indeed be interested in promoting all of them. But I guess you're right that we are interested in just the induction phi, so will look into that. 


================
Comment at: llvm/lib/Transforms/Scalar/LoopFlatten.cpp:617
+  SCEVExpander Rewriter(*SE, DL, "loopflatten");
+  SmallVector<WeakTrackingVH, 4> DeadInsts;
+
----------------
dmgreen wrote:
> What does DeadInstr contain after the calls to WidenIV? Should they be removed now, does that simplify things later on?
It's empty, because they are not "trivially" dead. I had a go at trying to clean it up, but it wasn't trivial, and needed a lot of code, for which we have other passes. You'll see some of the old phis in test cases, but they will cleaned up later by other passes.


================
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;
----------------
dmgreen wrote:
> Can this call findLoopComponents directly, or is there something in CanFlattenloopPair that would be needed?
Yeah, I looked at that, but there's a bit more going on there, e.g. it checks and sets the IV users. So, it's best and easiest just to run `CanFlattenLoopPair` again.


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

https://reviews.llvm.org/D90640