[PATCH] D70413: [musttail] Don't forward incoming registers over call site registers

Mon Nov 18 16:29:16 PST 2019

efriedma added inline comments.

================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:3901
+    // forwarding the original incoming value of AL. In these cases, the value
+    // passed at the call site wins.
     const auto &Forwards = X86Info->getForwardedMustTailRegParms();
----------------
rnk wrote:
> efriedma wrote:
> > This doesn't seem right... either we need to forward the value, or we don't.
> > 
> > We shouldn't be forwarding AL for Windows callling conventions; that convention for specifying the number of xmm arguments only applies to the "Linux" varargs ABI.
> So, I actually feel like musttail thunks really shouldn't assume a calling convention at all, they should simply preserve all non-CSR registers, including things like `RAX`, `R11`, `R10`, which are often scratch, and used for nest parameters, this CFG side parameter, and retpoline thunks. This makes it a much more general device for doing perfect forwarding. This change here is a step in that direction.
I guess my concern here, really, is that the "CCInfo.isAllocated" check seems sort of fragile?  But I guess it's a fundamental part of the way the forwarding works; we have argument registers that may or may not be used by a known argument.

It still seems confusing if getForwardedMustTailRegParms() contains a register we know doesn't actually need to forward.  And having a comment here describing that unnecessary functionality doesn't help.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70413