[PATCH] D114647: [TrivialDeadness] Introduce API separating two different usages

[Anna Thomas via Phabricator via llvm-commits]

anna added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/Local.cpp:435
     return false;
 
   if (!I->mayHaveSideEffects())
----------------
reames wrote:
> I would suggest an alternate implementation strategy here.
> 
> Leave the "along all paths" version unchanged.  
> 
> Add the new "along unused path" version, with explicitly early returns for a couple of cases, then default to calling the "along all paths" variant.
> 
> This should be lower risk and easier to review.  Then later, if you think it makes sense to invert the implementation, we can do that in a single NFC with tests for both in tree.  
I really like this suggestion. It shows the "added API". 



================
Comment at: llvm/lib/Transforms/Utils/Local.cpp:447
+  if (auto *Call = dyn_cast<CallBase>(I))
+    if (isMathLibCallNoop(Call, TLI))
+      return true;
----------------
mkazantsev wrote:
> This also looks like it can be made a part of `isGuaranteedToTransferExecutionToSuccessor`.
yes, can be done in a separate patch without increasing the scope here. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114647