[PATCH] D82566: [CodeMoverUtils] Make specific analysis dependent checks optional

[Whitney Tsang via Phabricator via llvm-commits]

Whitney added inline comments.


================
Comment at: llvm/include/llvm/Transforms/Utils/CodeMoverUtils.h:29
 bool isControlFlowEquivalent(const Instruction &I0, const Instruction &I1,
-                             const DominatorTree &DT,
-                             const PostDominatorTree &PDT);
+                             const DominatorTree *DT,
+                             const PostDominatorTree *PDT);
----------------
what's the reason why some arguments have nullptr as default value, and some doesn't?


================
Comment at: llvm/lib/Transforms/Utils/CodeMoverUtils.cpp:225
 bool llvm::isControlFlowEquivalent(const BasicBlock &BB0, const BasicBlock &BB1,
-                                   const DominatorTree &DT,
-                                   const PostDominatorTree &PDT) {
+                                   const DominatorTree *DT,
+                                   const PostDominatorTree *PDT) {
----------------
Is there an alternative way to check control flow equivalent without DT and PDT? If not, change it back to references.



================
Comment at: llvm/lib/Transforms/Utils/CodeMoverUtils.cpp:230
 
-  if ((DT.dominates(&BB0, &BB1) && PDT.dominates(&BB1, &BB0)) ||
-      (PDT.dominates(&BB0, &BB1) && DT.dominates(&BB1, &BB0)))
+  if ((DT->dominates(&BB0, &BB1) && PDT->dominates(&BB1, &BB0)) ||
+      (PDT->dominates(&BB0, &BB1) && DT->dominates(&BB1, &BB0)))
----------------
here you are dereferencing a pointer that can be a nullptr.


================
Comment at: llvm/lib/Transforms/Utils/CodeMoverUtils.cpp:319
+  if (!DT)
+    return false;
 
----------------
Actually if DI or PDT or DI, either one is nullptr, than this function never return true, so why not just check that in the beginning.


================
Comment at: llvm/lib/Transforms/Utils/CodeMoverUtils.cpp:365
+  if (!(DT && PDT))
+    return true;
+
----------------
return false


================
Comment at: llvm/lib/Transforms/Utils/CodeMoverUtils.cpp:373
+  if (!DI)
+    return true;
+
----------------
return false


================
Comment at: llvm/unittests/Transforms/Utils/CodeMoverUtilsTest.cpp:33
 static void run(Module &M, StringRef FuncName,
-                function_ref<void(Function &F, DominatorTree &DT,
-                                  PostDominatorTree &PDT, DependenceInfo &DI)>
+                function_ref<void(Function &F, DominatorTree *DT,
+                                  PostDominatorTree *PDT, DependenceInfo *DI)>
----------------
I prefer not to change any of the parameters from references to pointers, just change the call site.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82566