[PATCH] D69372: [X86][VARARG] Avoid spilling xmm vararg arguments.

[Alexey Lapshin via Phabricator via llvm-commits]

avl added a comment.

@rnk

> Anyway, it seems like you could do away with a fair amount of the complexity in this patch for handling guarding musttail forwarding by declaring them to be an "explicit FP use", since it is not, in general, possible for a perfectly forwarding thunk to know if XMM arguments have been used.

This effectively means that any program containing thunks could not be compiled for non-floating point environment even if it does not use fp at all. 
Taking into account that thunks are usually inserted by the compiler - that could be unexpected and hard to solve for the user. 
i.e. The program could not have floating point code at all but it would not be possible to compile it because of thunks.
I think it is not a good alternative.

> In retrospect, I think it may have been a mistake to repurpose varargs to indicate that all remaining argument registers should be preserved. Maybe we don't need to use a varargs function prototype to implement perfectly forwarding thunks. We ended up needing the "thunk" function attribute, so we could base it on that instead.

I agree that it is a mistake to use varargs for perfectly forwarding thunks purposes. Since it does not conform to current behavior and documented rules:

1. testcase from llvm/test/CodeGen/X86/musttail-varargs.ll:

  declare void @llvm.va_start(i8*) nounwind
  
  declare void(i8*, ...)* @get_f(i8* %this)
  
  define void @f_thunk(i8* %this, ...) {
    %ap = alloca [4 x i8*], align 16
    %ap_i8 = bitcast [4 x i8*]* %ap to i8*
    call void @llvm.va_start(i8* %ap_i8)
  
    %fptr = call void(i8*, ...)*(i8*) @get_f(i8* %this)
    musttail call void (i8*, ...) %fptr(i8* %this, ...)
    ret void
  }

It has "f_thunk" which has llvm.va_start. Thus it saves incoming xmm registers into va_start area :

  ; LINUX-NEXT:    testb %al, %al
  ; LINUX-NEXT:    je .LBB0_2
  ; LINUX-NEXT:  # %bb.1:
  ; LINUX-NEXT:    movaps %xmm0, {{[0-9]+}}(%rsp)
  ; LINUX-NEXT:    movaps %xmm1, {{[0-9]+}}(%rsp)
  ; LINUX-NEXT:    movaps %xmm2, {{[0-9]+}}(%rsp)
  ; LINUX-NEXT:    movaps %xmm3, {{[0-9]+}}(%rsp)
  ; LINUX-NEXT:    movaps %xmm4, {{[0-9]+}}(%rsp)
  ; LINUX-NEXT:    movaps %xmm5, {{[0-9]+}}(%rsp)
  ; LINUX-NEXT:    movaps %xmm6, {{[0-9]+}}(%rsp)
  ; LINUX-NEXT:    movaps %xmm7, {{[0-9]+}}(%rsp)
  ; LINUX-NEXT:  .LBB0_2:

if f_thunk would be called without setting AL it would work incorrectly.

2. calling "void (int, ...)" function without setting AL is AMD64 ABI violation.



3. https://llvm.org/docs/LangRef.html#id325

  The caller and callee prototypes must match. Pointer types of parameters or return types may differ in pointee type, but not in address space. The calling conventions of the caller and callee must match. All ABI-impacting function attributes, such as sret, byval, inreg, returned, and inalloca, must match.

  These rules clearly states that thunk signature and callee signature should agree on calling conventions. if thunk assumes that AL should be set then it is an error to not setting it.



-----

Probably we could fix implementation of perfectly forwarding thunks in an ABI compatible way ?


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

https://reviews.llvm.org/D69372