[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:353
+      if (!U->hasOneUse())
+        continue;
+      U = *U->user_begin();
----------------
Why continue, not returning false?


================
Comment at: llvm/lib/Transforms/Scalar/LoopFlatten.cpp:597
+  if (InnerType != OuterType ||
+      InnerType->getScalarSizeInBits() == MaxLegalSize) {
+    LLVM_DEBUG(dbgs() << "Can't widen the IV\n");
----------------
SjoerdMeijer wrote:
> 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.
As far as I understand, "legal" here just means "can fit into a single register", so 64bits on aarch64. There may already be IV's that are larger.


================
Comment at: llvm/lib/Transforms/Scalar/LoopFlatten.cpp:617
+  SCEVExpander Rewriter(*SE, DL, "loopflatten");
+  SmallVector<WeakTrackingVH, 4> DeadInsts;
+
----------------
SjoerdMeijer wrote:
> 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.
Ah OK. Is it possible to call DeleteDeadPhi's on the loops? It might simplify some of the "OrigPHIs" logic if they have already been removed.


================
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 ]
----------------
SjoerdMeijer wrote:
> dmgreen wrote:
> > 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.
> I have kept the test because this is my motivating example, and the IR input to loop flatten when it is generated from source. Skipping loop unswitch caused this to no longer trigger, and the test isn't that big, so I would prefer to keep it. If you insist, I will have another look though.
Yeah, I was mostly meaning to remove the blocks like for.cond1.preheader and for.cond1.preheader.preheader which is not part of the flattened loop. I think it should keep working on what remains if you manually edit it. If not and it doesn't keep working, but a big deal.


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

https://reviews.llvm.org/D90640