[PATCH] D83311: [CodeMoverUtils] Add optional data dependence checks using MSSA

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/CodeMoverUtils.cpp:228
+                             DepResult->isAnti()))
+                          return true;
+                        return false;
----------------
The if here doesn't add much I think. It would be simpler to just `return DepResult && DepResult->isOutput() || DepResult->isFlow() ||
                        DepResult->isAnti()`?


================
Comment at: llvm/lib/Transforms/Utils/CodeMoverUtils.cpp:233
+
+bool isDependenceSafe(Instruction &I, MemorySSAUpdater &MSSAU,
+                      SmallPtrSet<Instruction *, 10> InstsToCheck) {
----------------
I don't think there is a reason to pass MemorySSAUpdater here, as you don't modify the IR. Just pass MemorySSA directly.

Also, please add a comment what the logic behind the checks is (same for the DI version)


================
Comment at: llvm/lib/Transforms/Utils/CodeMoverUtils.cpp:245
+          return false;
+        bool IsFlowOrOutput = false;
+        bool IsAnti = false;
----------------
What we are doing here is basically checking if 2 instructions may alias, right? Given that, the variable names seem a bit confusing. Also, the function returns true if either IsFlowOrOutput or IsAnti is true. Could you just return true directly?


================
Comment at: llvm/lib/Transforms/Utils/CodeMoverUtils.cpp:368
   // Skip tests when we don't have PDT or DI
-  if (!PDT || !DI)
+  if (!PDT || !(DI || MSSAU))
     return false;
----------------
Does it make sense to even call this function if either of those are not available, i.e. if all those required wouldn't it make sense to assert that they are all provided or turn them into references?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83311