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

[Philip Reames via Phabricator via llvm-commits]

reames requested changes to this revision.
reames added a comment.
This revision now requires changes to proceed.

See inline comments.



================
Comment at: llvm/include/llvm/Transforms/Utils/Local.h:90
 /// isInstructionTriviallyDead would be true if the use count was 0.
-bool wouldInstructionBeTriviallyDead(Instruction *I,
-                                     const TargetLibraryInfo *TLI = nullptr);
+bool wouldInstructionBeTriviallyDeadIfUsesDead(
+    Instruction *I, const TargetLibraryInfo *TLI = nullptr);
----------------
I'd leave off the naming suffix here.  I think the original name is sufficiently clear.  

(i.e. "trivial dead" continues to mean "along all paths")


================
Comment at: llvm/include/llvm/Transforms/Utils/Local.h:98
+/// identifying instructions that can be sunk down to use(s).
+bool wouldInstructionBeTriviallyDeadOnPathsWithoutUse(
+    Instruction *I, const TargetLibraryInfo *TLI = nullptr);
----------------
Naming suggestion:
wouldInstructionBeTriviallyDeadAlongUnusedPath


================
Comment at: llvm/lib/Transforms/Utils/Local.cpp:435
     return false;
 
   if (!I->mayHaveSideEffects())
----------------
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.  


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114647