[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) {
+  return !std::any_of(InstsToCheck.begin(), InstsToCheck.end(),
+                      [&DI, &I](Instruction *CurInst) {
----------------
1. !any_of can change to none_of
2. use llvm::none_of instead of std::none_of
```
return none_of(InstsToCheck, [&DI, &I]
...
```


================
Comment at: llvm/lib/Transforms/Utils/CodeMoverUtils.cpp:250
+        if (isa<MemoryDef>(MemUseOrDef)) {
+          if (isa<MemoryUse>(DestMemUseOrDef) ||
+              isa<MemoryDef>(DestMemUseOrDef)) {
----------------
DestMemUseOrDef is of type MemoryUseOrDef, so it must be a MemoryUse of MemoryDef.


================
Comment at: llvm/unittests/Transforms/Utils/CodeMoverUtilsTest.cpp:582
+        EXPECT_TRUE(isSafeToMoveBefore(*LI2, *LI1, DT, &PDT, &DI, &MSSAU));
+        EXPECT_TRUE(isSafeToMoveBefore(*LI2, *LI1, DT, &PDT, nullptr, &MSSAU));
       });
----------------
bmahjour wrote:
> Please also add a check to make sure independent memory load/stores can be moved passed each other. For example, `%load2` should be able to move before the store to B.
> 
>            store i32 %load1, i32* %arrayidx_B, align 4
>            %load2 = load i32, i32* %arrayidx_A, align 4
Good idea. The test should include all four types of dependence, and all should be considered safe. Also make sure they are bidirectional, so check both move forward and move backward.


================
Comment at: llvm/unittests/Transforms/Utils/CodeMoverUtilsTest.cpp:513
                                        *CI_safecall->getNextNode(), DT, &PDT,
-                                       &DI));
+                                       &DI, &MSSAU));
+        EXPECT_TRUE(isSafeToMoveBefore(*CI_safecall->getPrevNode(),
----------------
RithikSharma wrote:
> Whitney wrote:
> > change all the existing ones to `&DI, nullptr))` to make sure you are testing `DI`.
> Sure but even when we give preference to DI?
> 
> ```
>  if (DI)
>     return isDependenceSafe(I, *DI, InstsToCheck);
>   else if (MSSAU)
>     return isDependenceSafe(I, *MSSAU, InstsToCheck);
> ```
Yes, because the code may change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83311