[PATCH] D73129: [LoopUnrollAndJam] Correctly update LoopInfo when unroll and jam more than 2-levels loop nests.

Wed Jan 22 08:31:46 PST 2020

Whitney marked 4 inline comments as done.
Whitney added inline comments.

================
Comment at: llvm/lib/Transforms/Utils/LoopUnrollAndJam.cpp:617
+  if (CompletelyUnroll)
+    LI->erase(L);
+
----------------
dmgreen wrote:
> I think the #ifndef NDEBUG checks below will re-use L after it has been deleted.
Good catch, will address this concern in https://reviews.llvm.org/D73204

================
Comment at: llvm/test/Transforms/LoopUnrollAndJam/loopnest.ll:6
+; CHECK-LABEL: foo
+; CHECK: %i = phi i64 [ 0, %entry ], [ %inc.i.3, %for.i.latch ]
+
----------------
jdoerfert wrote:
> dmgreen wrote:
> > Whitney wrote:
> > > jdoerfert wrote:
> > > > I would generate the check lines with the update_checks script. This doesn't test much.
> > > I tried to follow the same pattern as the other unroll and jam lit tests.  The main purpose of this test is to check if loop info verified correctly. I will update the test with check lines generated by use update_checks script.
> > Maybe just show control flow, like the test that was changed below?
> > Maybe just show control flow, like the test that was changed below?
> 
> The problem there is that the conditions are missing and you don't know where the loop bodies end up.
> 
> It's debatable what is best but this representation has some advantages when it comes to changes, it makes them automatically applicable and explicitly shows everything.
> 
> That said, I'm fine with manually cutting it down if ppl prefer that.
> 
How about I only keep the control flow, conditions, and phi nodes?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73129