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

[Whitney Tsang via Phabricator via llvm-commits]

Whitney added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/CodeMoverUtils.cpp:222
+                      SmallPtrSet<Instruction *, 10> InstsToCheck) {
+  if (!std::any_of(InstsToCheck.begin(), InstsToCheck.end(),
+                   [&DI, &I](Instruction *CurInst) {
----------------
```
  return !std::any_of(InstsToCheck.begin(), InstsToCheck.end(),
                   [&DI, &I](Instruction *CurInst) {
                     auto DepResult = DI.depends(&I, CurInst, true);
                     if (DepResult &&
                         (DepResult->isOutput() || DepResult->isFlow() ||
                          DepResult->isAnti()))
                       return true;
                     return false;
                   }));
```


================
Comment at: llvm/lib/Transforms/Utils/CodeMoverUtils.cpp:237
+                      SmallPtrSet<Instruction *, 10> InstsToCheck) {
+  if (!std::any_of(InstsToCheck.begin(), InstsToCheck.end(),
+                   [&MSSAU, &I](Instruction *CurInst) {
----------------
Below is the cleaner version of your code:
```
MemoryUseOrDef *MemUseOrDef = MSSAU.getMemorySSA()->getMemoryAccess(&I);
if (isa<MemoryDef>(MemUseOrDef))
  return false;
return !std::any_of(InstsToCheck.begin(), InstsToCheck.end(), &MSSAU, &I](Instruction *CurInst) {
  MemoryUseOrDef *MemUseOrDef = MSSAU.getMemorySSA()->getMemoryAccess(CurInst);
  return isa<MemoryDef>(MemUseOrDef);
});
```
But this code is very restrictive. It can be improved by considering the relationship between `I` and `CurInst`, e.g. if they don't access the same memory then it should still be safe. 


================
Comment at: llvm/unittests/Transforms/Utils/CodeMoverUtilsTest.cpp:513
                                        *CI_safecall->getNextNode(), DT, &PDT,
-                                       &DI));
+                                       &DI, &MSSAU));
+        EXPECT_TRUE(isSafeToMoveBefore(*CI_safecall->getPrevNode(),
----------------
change all the existing ones to `&DI, nullptr))` to make sure you are testing `DI`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83311