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

[Anna Thomas via Phabricator via llvm-commits]

anna marked 6 inline comments as done.
anna added inline comments.


================
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
----------------
mzolotukhin wrote:
> 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).
While running this through make-check, I realized the assertion should be the inverse: if the LHS fails, then we trip on the assert. Just like the assert on line 775 :)

```
assert(!(any_of(OtherExits,
                     [SuccBB](BasicBlock *EB) { return EB == SuccBB; }) ||
                SuccBB == LatchExit) &&
              "Breaks the definition of dedicated exits!");
```
 
I couldn't think of any other situations where we need to check this property about successors of the exit blocks (intent is clearer within this context). Still this is a valid property outside of DT, so moved it outside of DT.


================
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.
----------------
mzolotukhin wrote:
> This wording seems a bit strange. Is one of them a leftover from some editing?
yes, thanks!


================
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
----------------
mzolotukhin wrote:
> Could you please rearrange blocks, so that they stay in a (topo)logical order (like entry, preheader, header, latch, exit1, exit2, mergedexit)?
yes, done for all test cases. 


================
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
----------------
mzolotukhin wrote:
> Do we really need the shift in this test?
It can be any other operation. It's used to show the phi in the latchexit. 


https://reviews.llvm.org/D35304