[PATCH] D80643: [CodeMoverUtils] Move OrderedInstructions to CodeMoverUtils

[Whitney Tsang via Phabricator via llvm-commits]

Whitney added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/CodeMoverUtils.cpp:103
+}
+bool OrderedInstructions::dominates(const Instruction *InstA,
+                                    const Instruction *InstB) const {
----------------
No need, is not doing anything extra compare to `DominatorTree::dominates` defined in `lib/IR/Dominators.cpp`


================
Comment at: llvm/lib/Transforms/Utils/CodeMoverUtils.cpp:116
+  if (InstA->getParent() == InstB->getParent())
+    return localDominates(InstA, InstB);
+
----------------
As suggested by @nikic , change this call to `comesBefore`. And remove `OrderedInstructions::localDominates`.


================
Comment at: llvm/lib/Transforms/Utils/CodeMoverUtils.cpp:119
+  DomTreeNode *DA = DT->getNode(InstA->getParent());
+  DomTreeNode *DB = DT->getNode(InstB->getParent());
+}
----------------
Did you accidentally removed the line: `return DA->getLevel() < DB->getLevel();`?
Don't forget to build and test your change before updating your patch.


================
Comment at: llvm/lib/Transforms/Utils/CodeMoverUtils.cpp:333
   DT.updateDFSNumbers();
-  const bool MoveForward = OI.bfsLevelBefore(&I, &InsertPoint);
   Instruction &StartInst = (MoveForward ? I : InsertPoint);
----------------
Please update the base of your diff. It is already calling `domTreeLevelBefore` before this patch.


================
Comment at: llvm/unittests/Transforms/Utils/CodeMoverUtilsTest.cpp:652
 }
+TEST(CodeMoverUtils, DominanceTest) {
+  LLVMContext Ctx;
----------------
remove this test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80643