[PATCH] D107872: [SimplifyLibCalls] propagate tail/musttail/notail flags on CallInsts

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:2427-2429
+  // Must tail has invariants like trying not to refer to the callers stack
+  // frame via allocas. At the risk of LibCallSimplifier::optimizeCall()
+  // inserting such allocas, just skip libcall optimizing must tail calls.
----------------
nickdesaulniers wrote:
> There's probably a better way to express/word this. @efriedma please let me know what to replace here.
I'd probably say "Skip optimizing musttail calls so LibCallSimplifier::optimizeCall doesn't have to preserve those invariants."

Actually, to be on the safe side, probably best to also skip optimizing notail calls... I'm not what "notail" actually means on a libcall, if anything, but might as well be conservative.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:2440
+      assert(NewCI->isMustTailCall() &&
+             "replaced musttail call with non-musttail call");
+      (void)NewCI;
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107872