[PATCH] D56449: [UnrollRuntime] Support multi-exiting blocks to LatchExit

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev added inline comments.


================
Comment at: lib/Transforms/Utils/LoopUnrollRuntime.cpp:105
       // appropriate incoming values.
-      // TODO: This code assumes that the PrologExit (or the LatchExit block for
-      // prolog loop) contains only one predecessor from the loop, i.e. the
-      // PrologLatch. When supporting multiple-exiting block loops, we can have
-      // two or more blocks that have the LatchExit as the target in the
-      // original loop.
-      PHINode *NewPN = PHINode::Create(PN.getType(), 2, PN.getName() + ".unr",
+      PHINode *NewPN = PHINode::Create(PN.getType(), PrologExitPreds.size(), PN.getName() + ".unr",
                                        PrologExit->getFirstNonPHI());
----------------
Line too long.


================
Comment at: lib/Transforms/Utils/LoopUnrollRuntime.cpp:121
+        BasicBlock *LatchVBB = nullptr;
+        if (Instruction *I = dyn_cast<Instruction>(LatchV)) {
+          if (L->contains(I)) {
----------------
Could you please add more comments on this piece of logic? I don't quite understand it. Why do we make difference between phi latch inputs that come from inside the loop and from outside the loop?



================
Comment at: lib/Transforms/Utils/LoopUnrollRuntime.cpp:124
+            LatchV = VMap.lookup(I);
+            if (isa<Instruction>(LatchV))
+              LatchVBB = cast<Instruction>(LatchV)->getParent();
----------------
Maybe dyn_cast here and remove cast on line below?


================
Comment at: lib/Transforms/Utils/LoopUnrollRuntime.cpp:137
   // Make sure that created prolog loop is in simplified form
-  SmallVector<BasicBlock *, 4> PrologExitPreds;
   Loop *PrologLoop = LI->getLoopFor(PrologLatch);
----------------
That looks like it can go as a separate NFC.


================
Comment at: lib/Transforms/Utils/LoopUnrollRuntime.cpp:142
+          Value *PredIncomingValue = nullptr;
+          assert(DT && "We should have a DT at this point!");
+          // The incoming value should either be LatchV or
----------------
Why can we expect that? Other usage in code accounts for nullptr case.



Repository:
  rL LLVM

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

https://reviews.llvm.org/D56449