[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