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

[Philip Reames via Phabricator via llvm-commits]

reames added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:3790
+      auto *II = dyn_cast<IntrinsicInst>(UserI);
+      if (!II || !II->isLifetimeStartOrEnd())
+        return false;
----------------
nikic wrote:
> 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.
I landed a test inspired by this in 11a46b1 so that we don't forget it in the follow on.  It's annoying much more complicated than it looks as depending on the semantics of lifetime.start/end, this may actually be a valid transform.  But let's defer that conversion to that patch.  :) 


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

https://reviews.llvm.org/D116200