[PATCH] D82290: [CodeMoverUtils][WIP] Isolate checks strictly related to the code motion candidate instruction

[rithik sharma via Phabricator via llvm-commits]

RithikSharma marked 2 inline comments as done.
RithikSharma added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/CodeMoverUtils.cpp:308
+bool llvm::isSafeToMove(Instruction &I) {
+  if (!(isa<LoadInst>(I) || isa<StoreInst>(I) || isa<CallInst>(I) ||
+        isa<FenceInst>(I) || isa<CastInst>(I) || isa<UnaryOperator>(I) ||
----------------
Whitney wrote:
> RithikSharma wrote:
> > These conditions are inherited from LICM, is this a generalized safe assumption? Is there any very obvious exception to this?
> Not sure how this list is generated in LICM. Is it trying to exclude PHINode and terminators (e.g. BranchInst, SwitchInst, ...)? If so, then those was excluded in `isSafeToMoveBefore` originally. 
> Any test cases failed without this check? 
No failing test case yet and it does reflect the old `TermInst` and `PHINode` checks so it seems okay to me to keep this. 


================
Comment at: llvm/lib/Transforms/Utils/CodeMoverUtils.cpp:323
+      return reportInvalidCandidate(I, NotMovedDebugInfo);
+    if (CI->mayThrow())
+      return reportInvalidCandidate(I, NotMovedThrowCall);
----------------
Whitney wrote:
> This is covered in line 380.
Thanks, that check is dependent on DT. I can remove this from here but I'll wait until we decide on keeping DT and PDT as optional argument as then we may end up missing this check. (I'll also look into other such checks if we decide to make DT and PDT optional). 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82290