[PATCH] D93371: [DominatorTree] Add support for mixed pre/post CFG views.

[Jakub Kuderski via Phabricator via llvm-commits]

kuhar added inline comments.


================
Comment at: llvm/include/llvm/Analysis/MemorySSAUpdater.h:123
   /// Apply CFG updates, analogous with the DT edge updates.
-  void applyUpdates(ArrayRef<CFGUpdate> Updates, DominatorTree &DT);
+  /// If UpdateDTFirst is true, first update the DT with the same updates.
+  void applyUpdates(ArrayRef<CFGUpdate> Updates, DominatorTree &DT,
----------------
What happens when it's `false`? My immediate thought would be: will the DT remain unmodified and assumed up to date?


================
Comment at: llvm/include/llvm/Support/GenericDomTree.h:566
+      // applied.
+      SmallVector<UpdateType, 4> LocalUpdates;
+      for (auto &Update : Updates)
----------------
nit: drop the size parameter and let SmallVector pick it for you?
https://llvm.org/docs/ProgrammersManual.html#llvm-adt-smallvector-h


================
Comment at: llvm/lib/Analysis/MemorySSAUpdater.cpp:831
+      // deletes did not happen yet, hence the edges still exist.
+      DT.applyUpdates(Empty, RevDeleteUpdates);
+    } else {
----------------
Does this do anything if the update vector is Empty?


================
Comment at: llvm/lib/Transforms/Utils/LoopRotationUtils.cpp:507
           MSSAU->getMemorySSA()->verifyMemorySSA();
-      }
+      } else
+        DT->applyUpdates(Updates);
----------------
nit: add braces around the else body



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93371