[PATCH] D35304: [RuntimeLoopUnrolling] Update DomTree correctly when exit blocks have successors

[Michael Zolotukhin via Phabricator via llvm-commits]

mzolotukhin accepted this revision.
mzolotukhin added a comment.
This revision is now accepted and ready to land.

> Does this answer your question?

Yes, thank you for the detailed answer. The patch looks good to me then (please also find some minor comments/suggestions inline).

Thanks,
Michael



================
Comment at: lib/Transforms/Utils/LoopUnrollRuntime.cpp:768-773
+#ifdef EXPENSIVE_CHECKS
+         assert(any_of(OtherExits,
+                       [SuccBB](BasicBlock *EB) { return EB == SuccBB; }) ||
+                SuccBB == LatchExit &&
+                    "Breaks the definition of dedicated exits!");
+#endif
----------------
Are there any other situations where we might want to check this property? It might make sense to check it even before we checked if `DT` is available (under `#ifdef EXPENSIVE_CHECKS` of course).


================
Comment at: test/Transforms/LoopUnroll/runtime-loop-multiexit-dom-verify.ll:8-10
+; mergedexit is the merge of the loop exit blocks.
+
+; merged exit block has edges from loop exit blocks.
----------------
This wording seems a bit strange. Is one of them a leftover from some editing?


================
Comment at: test/Transforms/LoopUnroll/runtime-loop-multiexit-dom-verify.ll:23
+entry:
+  br i1 true,label %preheader, label %exit
+
----------------
I'd prefer to simplify `br i1 true` to unconditional branch or introduce a function parameter if you really need this branch to be conditional. Same applies to `undef`s used later in other places.


================
Comment at: test/Transforms/LoopUnroll/runtime-loop-multiexit-dom-verify.ll:25
+
+
+headerexit:                                              ; preds = %header
----------------
Redundant empty line.


================
Comment at: test/Transforms/LoopUnroll/runtime-loop-multiexit-dom-verify.ll:83-85
+preheader:                                              ; preds = %bb
+  %trip = zext i32 undef to i64
+  br label %header
----------------
Could you please rearrange blocks, so that they stay in a (topo)logical order (like entry, preheader, header, latch, exit1, exit2, mergedexit)?


================
Comment at: test/Transforms/LoopUnroll/runtime-loop-multiexit-dom-verify.ll:131
+latch:                                             ; preds = %header
+  %shft = ashr i64 %add.iv, 1
+  %cmp2 = icmp ult i64 %shft, %trip
----------------
Do we really need the shift in this test?


https://reviews.llvm.org/D35304