[PATCH] D45116: Don't inline branch funnels

[Reid Kleckner via Phabricator via llvm-commits]

rnk added a comment.

In https://reviews.llvm.org/D45116#1055897, @pcc wrote:

> > Maybe I forgot to check intrinsics, or the arguments that are part of the variadic pack in the verifier.
>
> I had to disable the verifier check for intrinsics because the check is not appropriate for all intrinsics, specifically this one.
>
> > Have you considered passing these extra parameters as an operand bundle, or do those not support variable numbers of operands?
>
> Something like that might work, but even if we were to represent the target list like that, that wouldn't be enough, because the backend would also need to know how to emit the other arguments as if they were being passed as part of a regular call.
>
> That is, not being able to inline arguments into an intrinsic is effectively a backend limitation that the inliner ought to respect. If that changes, we can change the inliner as well.


I think I see what you're saying. Because this intrinsic doesn't go through the normal ISel call lowering code paths, it doesn't have any logic for getting the rest of the arguments materialized into registers or stack slots. The musttail marking is used primarily to make sure that IR passes don't screw things up and move the intrinsic call out of tail position.

The current lowering seems a bit dangerous, because if some pass inserted some computation (__fentry calls maybe?) to a branch funnel function, you haven't created virtual registers connecting the incoming argument registers to the outgoing argument registers used by the tail call, and they won't be spilled and preserved. I think fixing that is just a matter of doing this stuff when you make your TAILJMP64d instructions:

  if (isVarArg && IsMustTail) {
    const auto &Forwards = X86Info->getForwardedMustTailRegParms();
    for (const auto &F : Forwards) {
      SDValue Val = DAG.getCopyFromReg(Chain, dl, F.VReg, F.VT);
      RegsToPass.push_back(std::make_pair(unsigned(F.PReg), Val));
    }
  }

... except you'd add MI copies and operands instead of DAG nodes.

In https://reviews.llvm.org/D45116#1056149, @vitalybuka wrote:

> So what should I do about this patch?


I think we shouldn't overgeneralize here. In the future, we can always add new intrinsics which don't work when we inline them. See llvm.local.escape. This seems like one more thing that we just don't know how to inline.

I think I would generalize `HasFrameEscape` to `HasUninlineableIntrinsic` and set that if you see a musttail call of this intrinsic. For example, I can imagine someone wanting to do guaranteed tail calls with some kind of statepoint intrinsic. We shouldn't block inlining that.


Repository:
  rL LLVM

https://reviews.llvm.org/D45116