[PATCH] D108521: [runtime] Move prolog/epilog block to a post-simplify strategy

[Anna Thomas via Phabricator via llvm-commits]

anna added a comment.

Hi Philip, couple of comments inline.



================
Comment at: llvm/lib/Transforms/Utils/LoopUnrollRuntime.cpp:949
+  // For unroll factor 2 remainder loop will have 1 iteration.
+  if (Count == 2 && DT && LI && SE) {
+    // TODO: This code could probably be pulled out into a helper function
----------------
Could you move this code above the `EXPENSIVE CHECKS` since there is DT updates here?
We can have full verification of `DT` in that case.


================
Comment at: llvm/lib/Transforms/Utils/LoopUnrollRuntime.cpp:952
+    // (e.g. breakLoopBackedgeAndSimplify) and reused in loop-deletion.
+    BasicBlock *RemainderLatch = remainderLoop->getLoopLatch();
+    SmallVector<BasicBlock*> RemainderBlocks(remainderLoop->getBlocks().begin(),
----------------
Pls add an assert that `RemainderLatch` exists (i.e. there is exactly one latch). 

There are couple of conditions required by `breakLoopBackedge`. Is `CloneLoopBlocks` guaranteed to take care of those conditions (preheader, single latch etc)?


================
Comment at: llvm/lib/Transforms/Utils/LoopUnrollRuntime.cpp:955
+                                             remainderLoop->getBlocks().end());
+    breakLoopBackedge(remainderLoop, *DT, *SE, *LI, nullptr);
+    remainderLoop = nullptr;
----------------
As a side-note, not relevant to this patch - we would need to add `MSSA` support if runtime unroll is used in an LPM with other passes that preserve `MSSA`.  


================
Comment at: llvm/lib/Transforms/Utils/LoopUnrollRuntime.cpp:963
+      for (BasicBlock::iterator I = BB->begin(), E = BB->end(); I != E;) {
+        Instruction *Inst = &*I++;
+        if (Value *V = SimplifyInstruction(Inst, {DL, nullptr, DT, AC}))
----------------
Purely stylistic: I usually prefer the iterator increment in the `for loop` as `++I` and then:
```
Instruction *Inst = &*I;
```


================
Comment at: llvm/lib/Transforms/Utils/LoopUtils.cpp:743
         // Transfer the metadata to the new branch instruction.
-        NewBI->copyMetadata(*BI, {LLVMContext::MD_loop, LLVMContext::MD_dbg,
+        NewBI->copyMetadata(*BI, {LLVMContext::MD_dbg,
                                   LLVMContext::MD_annotation});
----------------
Stray change? Can be landed separately?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108521