Fix ABI breakage with noimplicitfloat and varargs functions

[asl]

Summary

Currently we have a very nasty ABI breakage, because we will pass arguments in XMM registers, however the callee is expecting them on stack. Note that it is perfectly fine to spill XMM registers on stack even for functions with noimplicitfloat attribute: the spill is guarded by additional check and only happens if some XMM registers are live-ins with arguments.

[efriedma]

This makes sense, I think. Can we update LangRef and clang's command-line reference so it's clear this is actually the expected behavior?

[asl]

@efriedma The LangRef states that "This attributes disables implicit floating-point instructions". I believe that the definition of "implicit" is very vague here. And clang cmdline documentation does not explain "-mno-implicit-float" at all. What you'd suggest? :)

[efriedma]

The way the definition is written is a little backwards. Really, it means "The compiler will not generate any floating-point instructions unless there is an explicit floating-point operation."

Then maybe worth adding a quick explanation of what constructs count as an explicit floating-point operation. This falls into two categories. One, expressions where a value has floating-point type, vector type, or a struct type that contains a floating-point or vector type. Two, anywhere the platform's ABI for a call would require the use of float registers, possibly including varargs function definitions which don't have any explicit float arguments. Maybe also mention that this doesn't control the synthesis of calls to certain runtime functions, like memcpy or compiler-rt builtins.

The definition of a "floating-point instruction" is target-specific, and probably obvious to anyone who cares, so I think we can continue to use that term without defining it.

Note that on AArch64, there is no equivalent to x86's use of "al" for varargs calls.

[asl]

Note that on AArch64, there is no equivalent to x86's use of "al" for varargs calls.

Correct. Everywhere else we're using the same ABI regardless of this flag.

[salim.nasser]

Note that this change will expose bug 42219 ([X86_64] Variadic functions unconditionally spill %XMM registers). In that bug report I wrote:

"Specifically this behavior may become a problem for OS kernel code compiled with -no-implicit-float when bug 36507 is fixed (https://reviews.llvm.org/D62639).

Previously, variadic functions compiled with -no-implicit-float could safely be used even with the SSE unit disabled. If we fix 36507 without fixing the present bug, such code will lead to a crash at runtime due to reading XMM registers.

I understand that it is arguable whether or not that would be a bug. For example AArch64 variadic functions always unconditionally access the vector registers. But that is because the AArch64 ABI does not provide a way to avoid this. Whereas the X86_64 ABI *does*."

[inglorion]

I just uploaded D104001, which makes essentially the same change to the code, but adds some additional rationale and test coverage. I would be happy to have either change move forward. I've added everyone who has commented on this diff to D104001 as well.

llvm/lib/Target/X86/X86ISelLowering.cpp

static ArrayRef<MCPhysReg> get64BitArgumentXMMs(MachineFunction &MF,
   if (Subtarget.isCallingConvWin64(CallConv)) {
     // The XMM registers which might contain var arg parameters are shadowed
     // in their paired GPR.  So we only need to save the GPR to their home
     // slots.
     // TODO: __vectorcall will change this.
     return None;
   }
 
-  const Function &F = MF.getFunction();
-  bool NoImplicitFloatOps = F.hasFnAttribute(Attribute::NoImplicitFloat);
   bool isSoftFloat = Subtarget.useSoftFloat();
-  assert(!(isSoftFloat && NoImplicitFloatOps) &&
-         "SSE register cannot be used when SSE is disabled!");
-  if (isSoftFloat || NoImplicitFloatOps || !Subtarget.hasSSE1())
+  if (isSoftFloat || !Subtarget.hasSSE1())
     // Kernel mode asks for SSE to be disabled, so there are no XMM argument
     // registers.
     return None;
 
   static const MCPhysReg XMMArgRegs64Bit[] = {
     X86::XMM0, X86::XMM1, X86::XMM2, X86::XMM3,
     X86::XMM4, X86::XMM5, X86::XMM6, X86::XMM7
   };
SDValue X86TargetLowering::LowerFormalArguments(
   // the start of the first vararg value... for expansion of llvm.va_start. We
   // can skip this if there are no va_start calls.
   if (MFI.hasVAStart() &&
       (Is64Bit || (CallConv != CallingConv::X86_FastCall &&
                    CallConv != CallingConv::X86_ThisCall))) {
     FuncInfo->setVarArgsFrameIndex(MFI.CreateFixedObject(1, StackSize, true));
   }
 
-  // Figure out if XMM registers are in use.
-  assert(!(Subtarget.useSoftFloat() &&
-           F.hasFnAttribute(Attribute::NoImplicitFloat)) &&
-         "SSE register cannot be used when SSE is disabled!");
-
   // 64-bit calling conventions support varargs and register parameters, so we
   // have to do extra work to spill them in the prologue.
   if (Is64Bit && isVarArg && MFI.hasVAStart()) {
     // Find the first unallocated argument registers.
     ArrayRef<MCPhysReg> ArgGPRs = get64BitArgumentGPRs(CallConv, Subtarget);
     ArrayRef<MCPhysReg> ArgXMMs = get64BitArgumentXMMs(MF, CallConv, Subtarget);
     unsigned NumIntRegs = CCInfo.getFirstUnallocated(ArgGPRs);
     unsigned NumXMMRegs = CCInfo.getFirstUnallocated(ArgXMMs);
SDValue X86TargetLowering::LowerVAARG(SDValue Op, SelectionDAG &DAG) const {
   } else if (ArgVT.isInteger() && ArgSize <= 32 /*bytes*/) {
     ArgMode = 1;  // Argument passed in GPR64 register(s). Use gp_offset.
   } else {
     llvm_unreachable("Unhandled argument type in LowerVAARG");
   }
 
   if (ArgMode == 2) {
     // Sanity Check: Make sure using fp_offset makes sense.
-    assert(!Subtarget.useSoftFloat() &&
-           !(MF.getFunction().hasFnAttribute(Attribute::NoImplicitFloat)) &&
-           Subtarget.hasSSE1());
+    assert(!Subtarget.useSoftFloat() && Subtarget.hasSSE1());
   }
 
   // Insert VAARG_64 node into the DAG
   // VAARG_64 returns two values: Variable Argument Address, Chain
   SDValue InstOps[] = {Chain, SrcPtr, DAG.getConstant(ArgSize, dl, MVT::i32),
                        DAG.getConstant(ArgMode, dl, MVT::i8),
                        DAG.getConstant(Align, dl, MVT::i32)};
   SDVTList VTs = DAG.getVTList(getPointerTy(DAG.getDataLayout()), MVT::Other);

llvm/test/CodeGen/X86/noimplicitfp-varargs.ll

+; RUN: llc -mtriple=x86_64-unknown-unknown < %s | FileCheck %s
+%struct.__va_list_tag = type { i32, i32, i8*, i8* }
+
+define i32 @testvarargs(i8* nocapture readonly, ...) local_unnamed_addr #0 {
+; CHECK-LABEL: testvarargs
+; Ensure that xmm registers are indeed spilled
+; CHECK: testb %al, %al
+; CHECK: movaps  %xmm0, {{.*}}%rsp
+; CHECK: movaps  %xmm1, {{.*}}%rsp
+; CHECK: movaps  %xmm2, {{.*}}%rsp
+; CHECK: movaps  %xmm3, {{.*}}%rsp
+; CHECK: movaps  %xmm4, {{.*}}%rsp
+; CHECK: movaps  %xmm5, {{.*}}%rsp
+; CHECK: movaps  %xmm6, {{.*}}%rsp
+; CHECK: movaps  %xmm7, {{.*}}%rsp
+
+  %2 = alloca [1 x %struct.__va_list_tag], align 16
+  %3 = bitcast [1 x %struct.__va_list_tag]* %2 to i8*
+  %4 = getelementptr inbounds [1 x %struct.__va_list_tag], [1 x %struct.__va_list_tag]* %2, i64 0, i64 0
+  call void @llvm.va_start(i8* nonnull %3)
+  %5 = call i32 @vprintf(i8* %0, %struct.__va_list_tag* nonnull %4) #4
+  call void @llvm.va_end(i8* nonnull %3)
+  ret i32 %5
+}
+
+declare void @llvm.va_start(i8*) #2
+
+declare i32 @vprintf(i8* nocapture readonly, %struct.__va_list_tag*) local_unnamed_addr #3
+
+declare void @llvm.va_end(i8*) #2
+
+declare void @llvm.lifetime.end.p0i8(i64, i8* nocapture) #1
+
+attributes #0 = { noimplicitfloat nounwind ssp uwtable }
+attributes #2 = { nounwind }
+attributes #3 = { noimplicitfloat }
+attributes #4 = { noimplicitfloat }