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

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:3740
 
+
   // Do not sink static or dynamic alloca instructions. Static allocas must
----------------
nit: Spurious change


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:3772
+      return false;
+    auto *AI = dyn_cast<AllocaInst>(Dest->Ptr->stripPointerCasts());
+    if (!AI)
----------------
FWIW this could also use getUnderlyingObject() instead, as we don't care that the start of the alloca in particular is passed to the function, having an offset is fine.


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:3779
+    // account for cycles in doing so.
+    SmallVector<const User *, 4> AllocaUsers(AI->users());
+    while (!AllocaUsers.empty()) {
----------------
Unless something else already prevents this, I think you need a Visited set here to prevent potential infinite loops in unreachable code with cyclic references.


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:3790
+      auto *II = dyn_cast<IntrinsicInst>(UserI);
+      if (!II || !II->isLifetimeStartOrEnd())
+        return false;
----------------
Cast to IntrinsicInst isn't necessary here -- isLifetimeStartOrEnd() is actually defined on Instruction.


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:3790
+      auto *II = dyn_cast<IntrinsicInst>(UserI);
+      if (!II || !II->isLifetimeStartOrEnd())
+        return false;
----------------
nikic wrote:
> Cast to IntrinsicInst isn't necessary here -- isLifetimeStartOrEnd() is actually defined on Instruction.
This case is also untested. I think something we should check is that

```
lifetime.start
%x = call
lifetime.end

label:
use %x
```

doesn't get transformed into

```
lifetime.start
lifetime.end

label:
%x = call
use %x
```

I do believe the memory read check below protects against this, as we consider lifetime intrinsics to be reading memory as well.


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

https://reviews.llvm.org/D116200