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

[Philip Reames via Phabricator via llvm-commits]

reames added inline comments.


================
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
----------------
anna wrote:
> 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.
I'm happy to duplicate the expensive check if desired, but having it after manual DT update, and before well tested utilities which assume an up to date DT on entry makes sense to me.  


================
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(),
----------------
anna wrote:
> 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)?
Yes, these preconditions are implied.  Will add the requested assert.


================
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}))
----------------
anna wrote:
> Purely stylistic: I usually prefer the iterator increment in the `for loop` as `++I` and then:
> ```
> Instruction *Inst = &*I;
> ```
I would strongly prefer to retain this style.  It's idiomatic in the codebase.  


================
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});
----------------
anna wrote:
> Stray change? Can be landed separately?
Can be landed separately, will do.  


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108521