[PATCH] D107872: [SimplifyLibCalls] propagate tail flags on CallInsts

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:2440
+      assert(NewCI->isMustTailCall() &&
+             "replaced musttail call with non-musttail call");
+      (void)NewCI;
----------------
efriedma wrote:
> nickdesaulniers wrote:
> > efriedma wrote:
> > > nickdesaulniers wrote:
> > > > efriedma wrote:
> > > > > nickdesaulniers wrote:
> > > > > > efriedma wrote:
> > > > > > > I don't understand this assertion.  There are two possibilities for simplification:
> > > > > > > 
> > > > > > > 1. We replace a call with some existing value.
> > > > > > > 2. We replace a call with another new call.
> > > > > > > 
> > > > > > > For (1), this assertion doesn't make sense: we don't have a call.  For (2), we probably can't safely do the replacement in general; musttail has a bunch of restrictions, and the replacement call might not satisfy them.
> > > > > > These asserts were added in response to https://reviews.llvm.org/D107872#3059254.  I think they do make sense, from the perspective of guarding against `musttail` calls being transformed.
> > > > > > 
> > > > > > If the old call is musttail, and the recommended replacement is not also musttail, assert that the suggestion is probably broken.
> > > > > > 
> > > > > > It is anomalous if a must tail call was simplified to a value. (1)
> > > > > > 
> > > > > > It is anomalous if a must tail call was simplified to a new call that's also not must tail.
> > > > > > 
> > > > > > Though if we just avoid the call to `Simplifier.optimizeCall` above if `CI` is must tail, then these asserts become impossible and can be removed.
> > > > > I still think the assertion doesn't make sense.
> > > > > 
> > > > > In the "We replace a call with some existing value" case, consider, for example, memcpy.  We have an optimization that transforms memcpy->llvm.memcpy.  llvm.memcpy, doesn't have a return value, though, so we emit an llvm.memcpy, and return the value of the first operand. This operand may or may not be a call, and that call may or may not be a tail call.
> > > > For the case you describe, `With` will not be the `CallInst` to `llvm.memcpy`; it will be the destination operand of the original call to `memcpy`, so the `dyn_cast` will return `nullptr`. Thus the assertion is not checked at runtime for the case you describe.
> > > > 
> > > > The assertion is only checked when we replace a `CallInst` with another `CallInst` (with the new `CallInst` being what's returned from the call to `LibCallSimplifier::optimizeCall`).
> > > > 
> > > > To me, the assertion is a guard for future changes to LibCallSimplifier to ensure `copyFlags` is done safely in a manner that is done throughout this diff.
> > > > it will be the destination operand of the original call to memcpy
> > > 
> > > Yes.
> > > 
> > > > so the dyn_cast will return nullptr
> > > 
> > > I'm not following; is there some reason the destination can't be an instruction?
> > > > so the dyn_cast will return nullptr
> > > 
> > > I'm not following; is there some reason the destination can't be an instruction?
> > 
> > It //might// be an `Instruction`, but not necessarily a `CallInst`.  The `dyn_cast` is testing whether the returned `Value` is a `CallInst` and only checking that the tail call kind flag was propagate in that case.  In all other cases of different `Values` returned, the assertion is not checked.
> > 
> > ---
> > 
> > Or are you saying that the first operand to a `call` to `memcpy` could itself be a `CallInst`? Is that valid/possible to do in IR? ie.
> > ```
> > %bar = call i8* memcpy(call i8* @foo(), i8* %src, i64 %n)
> > ```
> > 
> > > This operand may or may not be a call, and that call may or may not be a tail call.
> > 
> > I guess I'm curious how can the first operand be a `call`? Perhaps:
> > ```
> > %dest = call i8* @foo()
> > %bar = call i8* @memcpy(i8* %dest, i8* %src, i64 %n)
> > ```
> > Does that mean that `%dest` is a `CallInst`?  If yes, then I indeed should remove the assert, but can you please confirm for me?
> In the following, `%dest` is a CallInst, yes.
> 
> ```
> %dest = call i8* @foo()
> %bar = call i8* @memcpy(i8* %dest, i8* %src, i64 %n)
> ```
Ok, let me add that as a test case, and drop the assertion. PTAL


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107872