[PATCH] D111611: [NFC] [LoopPeel] Change the way DT is updated for loop exits

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev requested changes to this revision.
mkazantsev added inline comments.
This revision now requires changes to proceed.


================
Comment at: llvm/lib/Transforms/Utils/LoopPeel.cpp:17
 #include "llvm/ADT/SmallVector.h"
+#include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/Statistic.h"
----------------
Pls remove this, DenseMap is already included on line 13.


================
Comment at: llvm/lib/Transforms/Utils/LoopPeel.cpp:567
+    ValueToValueMapTy &VMap, ValueToValueMapTy &LVMap, DomTreeUpdater &DTU,
+    SmallDenseMap<BasicBlock *, BasicBlock *> &LoopBlocksIDoms,
     LoopInfo *LI, ArrayRef<MDNode *> LoopLocalNoAliasDeclScopes) {
----------------
Pls make it const


================
Comment at: llvm/lib/Transforms/Utils/LoopPeel.cpp:578
 
+  SmallVector<DominatorTree::UpdateType, 8> DTUpdates;
+
----------------
You already have DTU. You can just call `insertEdge/deleteEdge` on it without any checks. They will not be applied immediately because you are using lazy update strategy. So no need for `DTUpdates` at all.


================
Comment at: llvm/lib/Transforms/Utils/LoopPeel.cpp:826
+  SmallDenseMap<BasicBlock *, BasicBlock *> LoopBlocksIDoms;
+  for (Loop::block_iterator BB = L->block_begin(), E = L->block_end(); BB != E;
+       ++BB)
----------------
```
for (auto *BB : L->blocks())
```



================
Comment at: llvm/lib/Transforms/Utils/LoopPeel.cpp:846
+      for (auto Exit : ExitEdgesSet)
+        DTU.applyUpdates({{DominatorTree::Insert,
+                           cast<BasicBlock>(LVMap[Exit.first]), Exit.second}});
----------------
You can just call `insertEdge` here. DTU with null tree handles it correctly. The application will be lazy. No need to check `hasDomTree`.


================
Comment at: llvm/lib/Transforms/Utils/LoopPeel.cpp:891
   // Finally DomtTree must be correct.
-  assert(DT->verify(DominatorTree::VerificationLevel::Fast));
+  if (DTU.hasDomTree())
+    assert(DTU.getDomTree().verify(DominatorTree::VerificationLevel::Fast));
----------------
Lazy DTU will only apply updates when it goes out of scope, or when it flushes. `simplifyLoop` below uses DT and needs it to be up to date. I'd suggest calling `DTU.flush()` here to apply all pending updates and leave assert as is.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111611