[PATCH] D116200: [instcombine] Allow sinking of calls with known writes to uses

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

The logic here looks correct to me, but I'm still a bit uncomfortable with this addition: The core transform here is that if you have a potentially written pointer argument that is not read after the call, then it is safe to sink it. However, we currently don't have a way to indicate that this argument is not going to be read lateron, so this ends up doing some ad-hoc analysis that involves scanning all alloca users. It will be hard to meaningfully extend this, because we can't (or at least shouldn't) add sophisticated alias analysis to InstCombine. On the other hand, DSE could easily determine this as a matter of course, i.e. mark an argument as dead even if the call cannot be dropped entirely.

That would be more involved though and I don't want perfect to be the enemy of good. Is there an immediate need for this? From D109917 <https://reviews.llvm.org/D109917> I got the impression that this was added purely for the sake of test coverage, and not because of a performance issue encountered in the wild.



================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:3767
+    while (!AllocaUsers.empty()) {
+      auto *UserI = dyn_cast<Instruction>(AllocaUsers.pop_back_val());
+      if (isa<BitCastInst>(UserI) || isa<GetElementPtrInst>(UserI)) {
----------------
dyn_cast -> cast


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:3768
+      auto *UserI = dyn_cast<Instruction>(AllocaUsers.pop_back_val());
+      if (isa<BitCastInst>(UserI) || isa<GetElementPtrInst>(UserI)) {
+        AllocaUsers.append(UserI->user_begin(), UserI->user_end());
----------------
Should handle AddrSpaceCastInst as well.


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

https://reviews.llvm.org/D116200