[PATCH] D33001: [RuntimeUnrolling] Add logic for loops with multiple exit blocks

[Evgeny Stupachenko via Phabricator via llvm-commits]

evstupac added a comment.

Hi Anna,

Thank for your contribution.
I've made some inline comments. Please take a look.

Thanks,
Evgeny



================
Comment at: lib/Transforms/Utils/LoopUnrollRuntime.cpp:496
+  if (!Exit && UnrollRuntimeMultiExit) {
+    assert(UseEpilogRemainder && "Multi exit unrolling is currently supported "
+                                 "unrolling with epilog remainder only!");
----------------
How about passing "UnrollRuntimeMultiExit" as a parameter to UnrollRuntimeLoopRemainder" and introducing dependency like:
UseEpilogRemainder |= UnrollRuntimeMultiExit for loops with multi exits and
UnrollRuntimeMultiExit &= UseEpilogRemainder if UseEpilogRemainder is set by user?





================
Comment at: lib/Transforms/Utils/LoopUnrollRuntime.cpp:508
+        continue;
+      if (BB->getTerminator()->getNumSuccessors())
+        return false;
----------------
Could you please add a comment here?


================
Comment at: lib/Transforms/Utils/LoopUnrollRuntime.cpp:691
 
+  // At this stage, the loop blocks are cloned for the prolog/epilog. Clone the
+  // other exit blocks as well for the epilog, and update the Vmap and dominator
----------------
Maybe it's ok, but according to the comment (line 685) we clone extra blocks right after inserting previous into function. The order needs an additional comment.

I'd recommend adding this part to "CloneLoopBlocks".


================
Comment at: lib/Transforms/Utils/LoopUnrollRuntime.cpp:698
+    NewBB->setName(BB->getName() + ".epil");
+    for (BasicBlock::iterator II = BB->begin(), IE = BB->end(); II != IE;
+         ++II) {
----------------
This should be more compact:
for (Instruction &II : *BB)


https://reviews.llvm.org/D33001