[PATCH] D70049: [CodeMoverUtils] Added an API to check if an instruction can be safely moved before another instruction.

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added inline comments.


================
Comment at: llvm/include/llvm/Transforms/Utils/CodeMoverUtils.h:29-31
+bool IsControlFlowEquivalent(const Instruction &I0, const Instruction &I1,
+                             const DominatorTree &DT,
+                             const PostDominatorTree &PDT);
----------------
[style] Start function names with lower case letters.


================
Comment at: llvm/lib/Analysis/PostDominators.cpp:58
+  // Loop through the basic block until we find I1 or I2.
+  BasicBlock::const_iterator I = BB1->begin();
+  for (; &*I != I1 && &*I != I2; ++I)
----------------
[serious] Should ensure that not both of the instructions are PHIs. These do not (post-)dominate each other even if one follows the other. See https://llvm.org/PR26718.


================
Comment at: llvm/lib/Transforms/Utils/CodeMoverUtils.cpp:49
+                              DependenceInfo &DI) {
+  assert(&I != &InsertPoint && "Cannot move itself before itself");
+
----------------
[suggestion] Could just return `false` in this case. Even if this is a wrong use of the interface, callers should not do anything.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D70049