isSafeToMoveBefore uses Dependence Info to check for flow/anti/output dependence, this patch adds alternative checks using MSSA.
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
llvm/lib/Transforms/Utils/CodeMoverUtils.cpp | ||
---|---|---|
221 | 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; })); | |
236 | 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. | |
llvm/unittests/Transforms/Utils/CodeMoverUtilsTest.cpp | ||
512–515 | change all the existing ones to &DI, nullptr)) to make sure you are testing DI. |
llvm/unittests/Transforms/Utils/CodeMoverUtilsTest.cpp | ||
---|---|---|
512–515 | Sure but even when we give preference to DI? if (DI) return isDependenceSafe(I, *DI, InstsToCheck); else if (MSSAU) return isDependenceSafe(I, *MSSAU, InstsToCheck); |
llvm/unittests/Transforms/Utils/CodeMoverUtilsTest.cpp | ||
---|---|---|
581 | 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 |
llvm/lib/Transforms/Utils/CodeMoverUtils.cpp | ||
---|---|---|
227 | The if here doesn't add much I think. It would be simpler to just `return DepResult && DepResult->isOutput() || DepResult->isFlow() || DepResult->isAnti()`? | |
232 | 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) | |
244 | 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? | |
358 | 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? |
llvm/lib/Transforms/Utils/CodeMoverUtils.cpp | ||
---|---|---|
221 |
return none_of(InstsToCheck, [&DI, &I] ... | |
249 | DestMemUseOrDef is of type MemoryUseOrDef, so it must be a MemoryUse of MemoryDef. | |
llvm/unittests/Transforms/Utils/CodeMoverUtilsTest.cpp | ||
512–515 | Yes, because the code may change. | |
581 | 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. |
llvm/lib/Transforms/Utils/CodeMoverUtils.cpp | ||
---|---|---|
227 | Thanks, yeah I should directly return it. I missed it in this diff as well. I'll update in the next diff. | |
232 | Acknowledged and updated to MemorySSA instead of MemorySSAUpdater. Will add the required comments in the next diff. | |
358 | I'm sorry, I didn't understand. We need at least DI or MSSA to find dependency. |
llvm/lib/Transforms/Utils/CodeMoverUtils.cpp | ||
---|---|---|
358 | I meant does it make sense to call this function without PDT == nullptr for example? It seems like it is kind of required here, right? |
llvm/lib/Transforms/Utils/CodeMoverUtils.cpp | ||
---|---|---|
358 | Got it, why is PDT not a reference if it is required, right? Most code motion clients example LICM don't have PDT so until we find a way to prove control flow equivalence with some other analysis we need to keep the !PDT check but we did changed PDT into pointer as we will be expecting nullptr in near future. |
llvm/lib/Transforms/Utils/CodeMoverUtils.cpp | ||
---|---|---|
259 | I don't understand why we need this part: || InstLastDefMA == CurInstDefMA. Wouldn't it suffice to check the reaching def of the use against the instruction being moved (the first part of this condition) ? Do you have a test that would fail if you remove it? | |
264 | Do we really need InstDefMA == CurInstLastDefMA ||? See my comment above. |
llvm/lib/Transforms/Utils/CodeMoverUtils.cpp | ||
---|---|---|
264 | This is needed for cases like store 1, ptr x = load ptr store 0, ptr check if x is safe to move after store 0, ptr. // Anti forward dependency EXPECT_FALSE(isSafeToMoveBefore(*LoadA1, *StoreB1, DT, &PDT, &DI)); the defining access of x is store 1, ptr, the getClobberingMemoryAccess of store 0, ptr is x. |
llvm/lib/Transforms/Utils/CodeMoverUtils.cpp | ||
---|---|---|
259 | Yes we have a failure if we remove this, isSafeToMoveBefore(*StoreA2, *LoadA1, DT, &PDT, nullptr, &MSSA) |
llvm/lib/Transforms/Utils/CodeMoverUtils.cpp | ||
---|---|---|
264 | That still won't work if the first store is after the load or is absent all together. I don't think there is a way to get x from store 0, ptr because there is no def-use relationship between them. This highlights the need to use Alias Analysis instead or in combination with MSSA. I see that current LICM has similar issues, for example see comment in pointerInvalidatedByLoopWithMSSA(). I suppose improvements can be made in steps. Currently LICM uses an over-approximation to deal with this situation by looking for any definitions that are locally dominated by the sinking use. Perhaps we can start with that and gradually add aliasing queries into the equation. |
llvm/lib/Transforms/Utils/CodeMoverUtils.cpp | ||
---|---|---|
255 | We also need to check for InstLastDefMA == CurInstLastDefMA in case both definitions are clobbered by the same memory phi. |
This can be closed in favor of https://reviews.llvm.org/D84589. Please migrate the tests and close the review. Thanks.