isSafeToMoveBefore uses Dependence Info to check for flow/anti/output dependence, this patch adds alternative checks using Alias Analysis.
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
llvm/lib/Transforms/Utils/CodeMoverUtils.cpp | ||
---|---|---|
230 | Please add a description for each isDependenceSafe(). And declare them as static. | |
231 | SmallPtrSet<Instruction *, 10> -> SmallPtrSetImpl<Instruction *> Same for the other isDependenceSafe(). | |
258 | check this at line 251. if (AA.isNoAlias(MemLoc, DestMemLoc)) return false; | |
llvm/unittests/Transforms/Utils/CodeMoverUtilsTest.cpp | ||
46 | Does DataLayout DL(""); work? |
llvm/unittests/Transforms/Utils/CodeMoverUtilsTest.cpp | ||
---|---|---|
46 | Thanks, yes it works. |
llvm/lib/Transforms/Utils/CodeMoverUtils.cpp | ||
---|---|---|
252 | Test case pending. |
llvm/lib/Transforms/Utils/CodeMoverUtils.cpp | ||
---|---|---|
264 | Inst (from the list of instructions being checked) may also be a call. |
llvm/lib/Transforms/Utils/CodeMoverUtils.cpp | ||
---|---|---|
249 | Not all aliasing implications are visible through the arguments. AAResults contains a number of overloads specifically designed to handle function calls. Did you look into using the following instead? ModRefInfo getModRefInfo(const CallBase *Call, const MemoryLocation &Loc); ModRefInfo getModRefInfo(Instruction *I, const CallBase *Call); ModRefInfo getModRefInfo(const CallBase *Call1, const CallBase *Call2); |
Are there plans on moving some transforms to use this with AA? I think it would be good to have a concrete plan to make use of this, otherwise I am worried that the code will miss out on large real-world test coverage.
This was inspired from LICM, I'm trying to test this part by manually plugging this into LICM.
llvm/lib/Transforms/Utils/CodeMoverUtils.cpp | ||
---|---|---|
252 | Is the test cases for call going to be added in this patch? | |
260 | You don't need to variable IsSafe anymore, as you return false right away if IsSafe is false. And you can return true at the end of the function instead. | |
264 |
I don't understand this comment. | |
280 | Is it safe to check this first if the memory locations are not None? auto DestMemLoc = MemoryLocation::get(Inst); if (MemLoc.hasValue() && DestMemLoc.hasValue() && AA.isNoAlias(MemLoc, DestMemLoc)) return false; ModRefInfo Result = AA.getModRefInfo(Inst, MemLoc); ModRefInfo DestResult = AA.getModRefInfo(&I, DestMemLoc); if (CallBase *CBDest = dyn_cast<CallBase>(Inst)) { if (!MemLoc.hasValue()) Result = DestResult = AA.getModRefInfo(&I, CBDest); if (CallBase *CBSrc = dyn_cast<CallBase>(&I)) if (!DestMemLoc.hasValue()) DestResult = AA.getModRefInfo(Inst, CBSrc); | |
433 | why remove isSafeToSpeculativelyExecute? |
Thanks for the reviews @Whitney , I have updated the patch with the changes.
llvm/lib/Transforms/Utils/CodeMoverUtils.cpp | ||
---|---|---|
252 | Test cases are present in https://reviews.llvm.org/D84776 |
llvm/lib/Transforms/Utils/CodeMoverUtils.cpp | ||
---|---|---|
287 | Two comments here:
|
llvm/lib/Transforms/Utils/CodeMoverUtils.cpp | ||
---|---|---|
287 |
I have made the required changes. |
llvm/lib/Transforms/Utils/CodeMoverUtils.cpp | ||
---|---|---|
250 | I think the call itself should also go into the MemLocs vector. |
llvm/lib/Transforms/Utils/CodeMoverUtils.cpp | ||
---|---|---|
250 | CallInst returns none memory location, which won't be useful to find dependence info and we may have to skip it so I did not add it into the MemLocs in the first place. |
llvm/lib/Transforms/Utils/CodeMoverUtils.cpp | ||
---|---|---|
250 | Then we have a problem if the memory accesses of a function are not represented through their arguments. For example consider: int a = 0; void foo() { a = 1; } int main() { foo(); a = 3; return a; } In the case above a = 3 should not be allowed to move before the call to foo(). If this information is not available through AliasAnalysis, then we have to check for it separately. One way to solve it is to look at the underlying object referenced by I and Inst, and if any of them is a global and one of the intervening instructions is a call that may have side-effect, then return false. |
Please add a description for each isDependenceSafe(). And declare them as static.