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

[rithik sharma via Phabricator via llvm-commits]

RithikSharma marked 3 inline comments as done.
RithikSharma added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/CodeMoverUtils.cpp:228
+                             DepResult->isAnti()))
+                          return true;
+                        return false;
----------------
fhahn wrote:
> The if here doesn't add much I think. It would be simpler to just `return DepResult && DepResult->isOutput() || DepResult->isFlow() ||
>                         DepResult->isAnti()`?
Thanks, yeah I should directly return it. I missed it in this diff as well. I'll update in the next diff. 


================
Comment at: llvm/lib/Transforms/Utils/CodeMoverUtils.cpp:233
+
+bool isDependenceSafe(Instruction &I, MemorySSAUpdater &MSSAU,
+                      SmallPtrSet<Instruction *, 10> InstsToCheck) {
----------------
fhahn wrote:
> 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)
Acknowledged and updated to MemorySSA instead of MemorySSAUpdater. Will add the required comments in the next diff. 


================
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;
----------------
fhahn wrote:
> 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?
I'm sorry, I didn't understand. We need at least DI or MSSA to find dependency. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83311