[PATCH] D70049: [CodeMoverUtils] Added an API to check if an instruction can be safely moved before another instruction.
Bardia Mahjour via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Nov 14 14:45:47 PST 2019
bmahjour added inline comments.
================
Comment at: llvm/lib/Transforms/Utils/CodeMoverUtils.cpp:134
+ // never return, from I to InsertPoint.
+ InstsToCheck.insert(&I);
+ if (std::any_of(InstsToCheck.begin(), InstsToCheck.end(), [](Instruction *I) {
----------------
None of these checks are required for simple instructions (such as `add` and `icmp` which are used in the test bellow). I think we should only check for throw/sync/noreturn if `isSafeToSpeculativelyExecute(I)` returns false.
================
Comment at: llvm/lib/Transforms/Utils/CodeMoverUtils.cpp:157
+ [&DI, &I](Instruction *CurInst) {
+ CurInst->dump();
+ auto DepResult = DI.depends(&I, CurInst, true);
----------------
remove the dump or guard it.
================
Comment at: llvm/unittests/Transforms/Utils/CodeMoverUtilsTest.cpp:71
+ "entry:\n"
+ " %N = add nsw i64 %N_, 1\n"
+ " call void @safecall()\n"
----------------
Please see my comment above. I think we should use an instruction with potential side-effect to really test what moves across safecalls and what doesn't move across unsafe calls. We should also test that instructions without side-effect can be moved across function calls as long as def-use dependency is preserved.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D70049/new/
https://reviews.llvm.org/D70049
More information about the llvm-commits
mailing list