[PATCH] D107872: [SimplifyLibCalls] propagate tail/musttail/notail 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:
> 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.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp:1162
 Value *LibCallSimplifier::optimizeMemPCpy(CallInst *CI, IRBuilderBase &B) {
+  if (CI->isMustTailCall())
+    return nullptr;
----------------
efriedma wrote:
> xbolva00 wrote:
> > Is it worth it at all?
> > 
> > I would just bail out libcall simplifier for musttail CIs
> I agree; musttail calls should be rare enough that it isn't a big deal to just skip libcall simplification completely
So just completely skipping the call to `LibCallSimplifier::optimizeCall` from `InstCombinerImpl::tryOptimizeCall` if the call is must tail?  That's going to require me to rewrite a lot of the newly added test cases here; please confirm we're in mutual understanding before I go and make such invasive changes to this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107872