Make variable argument intrinsics behave correctly in a Win64 CC function.

[cdavis5x]

Summary

This change makes the variable argument intrinsics, llvm.va_start and
llvm.va_copy, and the va_arg instruction behave as they do on Windows
inside a CallingConv::X86_64_Win64 function. It's needed for a Clang patch
I have to add support for GCC's __builtin_ms_va_list constructs.

[cdavis5x]

Adding Owen to review the changes to SelectionDAG.

[resistor]

The generic SelectionDAG changes look fine to me in principle. I'm not familiar with the X86 or MS-specific lowering.

[cdavis5x]

@resistor wrote:

The generic SelectionDAG changes look fine to me in principle. I'm not familiar with the X86 or MS-specific lowering.

I know. I just wanted to make sure the SelectionDAG changes were fine.

[eugenis]

lib/Transforms/Instrumentation/MemorySanitizer.cpp

1924

I'd prefer if this was done in CreateVarArgHelper() by selecting VarArgNoOpHelper for Win64.

[cdavis5x]

Create a no-op varargs handler in msan for a Win64 ABI function.

[cdavis5x]

lib/Transforms/Instrumentation/MemorySanitizer.cpp

1924

Done. (I didn't do this for Win64 targets, only for functions with the Win64 CC.)

[eli.friedman]

lib/Target/X86/X86ISelLowering.cpp

10848

Checking DAG.getMachineFunction().getFunction()->getCallingConv() is fine for va_start, but it isn't correct for va_arg or va_copy: a function using the Win64 calling convention can pass its va_list to a function using the AMD calling convention, or vice versa. And even if nobody writes code like that, the optimizer could potentially introduce such constructs.

[eugenis]

MSan change looks good.

[cdavis5x]

Attempt to address Eli's concerns.

• Make Win64 ABI variable argument intrinsics work even in a SysV ABI function.
• Sink the Win64 ABI checks back into the visitVA*Inst functions in msan. (Sorry, but I had to do it that way because of the first change.)

[cdavis5x]

lib/Target/X86/X86ISelLowering.cpp

10848

I've attempted to address this now by having the lowering code for va_arg and va_copy try to do Win64-style lowering based on the types of the arguments, but I'm not sure if this is the right way to go.

[eugenis]

lib/Transforms/Instrumentation/MemorySanitizer.cpp

1942

This looks very fragile.
Why can't you decide this based on the calling convention of the instrumented function?

[eugenis]

lib/Transforms/Instrumentation/MemorySanitizer.cpp

1942

OK, va_list may be from a function with a different calling convention.
I don't see a better way to detect this, but... how does va_copy work in this case? It gets plain i8* as an argument.

[cdavis5x]

lib/Transforms/Instrumentation/MemorySanitizer.cpp

1942

Remember how LLVM IR works--it's single-static assignment. All instructions are values. The IR for a Win64 ABI va_copy typically looks something like this:

%ap = alloca i8*, align 8
%cp = alloca i8*, align 8
%ap1 = bitcast i8** %ap to i8*
%cp1 = bitcast i8** %cp to i8*
call void @llvm.va_copy(i8* %cp1, i8* %ap1)

So, I dyn_cast the argument to an llvm::BitCastInst (and hope that the optimizer didn't make the bitcast go away--yes, this is very fragile, but I can't think of any better way to do this), and check if the source type is a i8** (the argument to the bitcast is a pointer to the va_list itself). If it is, we assume it's a Win64 ABI va_copy, else, a System V ABI va_copy.

Now that I think about it, it might be better if I just add a new intrinsic (llvm.ms_va_copy, maybe?) to handle Win64 ABI va_copy calls, but when I tried to do it this way, I couldn't work out how to use the generic lowering code (that I pulled out of LegalizeDAG) to lower it. (In retrospect, I should probably have asked about it earlier on the list.)

[cdavis5x]

Big belated update:

• Rebase onto rL243284.
• Go back to checking if the containing function uses the Win64 ABI. Opaque pointer types make checking the va_list type difficult.
• Don't do the "don't save XMM registers" dance in a variadic Win64 ABI function on a SysV platform. (This was something I missed last time.)

[rnk]

Looks good to me. Owen also mentioned that the SDAG changes were fine. They are mostly code motion.

[cdavis5x]

Gut target OS assertion from LowerVAARG.

I tried very hard not to change its current behavior other than inside
a Win64 ABI function.

I know you said it was OK to commit, but I want to run this one last
change by you guys before I go ahead.

[rnk]

lib/Target/X86/X86ISelLowering.cpp

10846–10848

I'm assuming this extra condition was needed for some test case? Should we change isCallingConvWin64 instead maybe?

[cdavis5x]

lib/Target/X86/X86ISelLowering.cpp

10846–10848

This was me trying really hard--too hard?--not to change the existing behavior. Basically, prior to this change on Windows, we always had SelectionDAG expand va_arg instructions assuming a char * pointer. Now that I've reread the isCallingConvWin64 function source, I realize this isn't really necessary.

[cdavis5x]

Just call X86Subtarget::isCallingConvWin64 in LowerVAARG.

Don't bother checking if the function has a System V calling
convention on Windows; just use X86Subtarget::isCallingConvWin64.
That should be good enough for our purposes, since we're just going
to manually lower __builtin_va_arg on a __builtin_ms_va_list
anyhow. (I only added lowering of the va_arg instruction for
completeness here.)

[rnk]

lgtm

[cdavis5x]

Closing manually because Phabricator for some reason didn't close it for me...

include/llvm/CodeGen/SelectionDAG.h

#endif
 
   /// getMDNode - Return an MDNodeSDNode which holds an MDNode.
   SDValue getMDNode(const MDNode *MD);
 
   /// getShiftAmountOperand - Return the specified value casted to
   /// the target's desired shift amount type.
   SDValue getShiftAmountOperand(EVT LHSTy, SDValue Op);
 
+  /// \brief Expand the given \c ISD::VAARG node as the Legalize pass would.
+  SDValue expandVAArg(SDNode *Node);
+
+  /// \brief Expand the given \c ISD::VACOPY node as the Legalize pass would.
+  SDValue expandVACopy(SDNode *Node);
+
   /// UpdateNodeOperands - *Mutate* the specified node in-place to have the
   /// specified operands.  If the resultant node already exists in the DAG,
   /// this does not modify the specified node, instead it returns the node that
   /// already exists.  If the resultant node does not exist in the DAG, the
   /// input node is returned.  As a degenerate case, if you specify the same
   /// input operands as the node already has, the input node is returned.
   SDNode *UpdateNodeOperands(SDNode *N, SDValue Op);
   SDNode *UpdateNodeOperands(SDNode *N, SDValue Op1, SDValue Op2);

lib/CodeGen/SelectionDAG/LegalizeDAG.cpp

False = DAG.getNode(ISD::FP_TO_SINT, dl, NVT,
                         DAG.getNode(ISD::FSUB, dl, VT,
                                     Node->getOperand(0), Tmp1));
     False = DAG.getNode(ISD::XOR, dl, NVT, False,
                         DAG.getConstant(x, NVT));
     Tmp1 = DAG.getSelect(dl, NVT, Tmp2, True, False);
     Results.push_back(Tmp1);
     break;
   }
-  case ISD::VAARG: {
-    const Value *V = cast<SrcValueSDNode>(Node->getOperand(2))->getValue();
-    EVT VT = Node->getValueType(0);
-    Tmp1 = Node->getOperand(0);
-    Tmp2 = Node->getOperand(1);
-    unsigned Align = Node->getConstantOperandVal(3);
-
-    SDValue VAListLoad = DAG.getLoad(TLI.getPointerTy(), dl, Tmp1, Tmp2,
-                                     MachinePointerInfo(V),
-                                     false, false, false, 0);
-    SDValue VAList = VAListLoad;
-
-    if (Align > TLI.getMinStackArgumentAlignment()) {
-      assert(((Align & (Align-1)) == 0) && "Expected Align to be a power of 2");
-
-      VAList = DAG.getNode(ISD::ADD, dl, VAList.getValueType(), VAList,
-                           DAG.getConstant(Align - 1,
-                                           VAList.getValueType()));
-
-      VAList = DAG.getNode(ISD::AND, dl, VAList.getValueType(), VAList,
-                           DAG.getConstant(-(int64_t)Align,
-                                           VAList.getValueType()));
-    }
-
-    // Increment the pointer, VAList, to the next vaarg
-    Tmp3 = DAG.getNode(ISD::ADD, dl, VAList.getValueType(), VAList,
-                       DAG.getConstant(TLI.getDataLayout()->
-                          getTypeAllocSize(VT.getTypeForEVT(*DAG.getContext())),
-                                       VAList.getValueType()));
-    // Store the incremented VAList to the legalized pointer
-    Tmp3 = DAG.getStore(VAListLoad.getValue(1), dl, Tmp3, Tmp2,
-                        MachinePointerInfo(V), false, false, 0);
-    // Load the actual argument out of the pointer VAList
-    Results.push_back(DAG.getLoad(VT, dl, Tmp3, VAList, MachinePointerInfo(),
-                                  false, false, false, 0));
+  case ISD::VAARG:
+    Results.push_back(DAG.expandVAArg(Node));
     Results.push_back(Results[0].getValue(1));
     break;
-  }
-  case ISD::VACOPY: {
-    // This defaults to loading a pointer from the input and storing it to the
-    // output, returning the chain.
-    const Value *VD = cast<SrcValueSDNode>(Node->getOperand(3))->getValue();
-    const Value *VS = cast<SrcValueSDNode>(Node->getOperand(4))->getValue();
-    Tmp1 = DAG.getLoad(TLI.getPointerTy(), dl, Node->getOperand(0),
-                       Node->getOperand(2), MachinePointerInfo(VS),
-                       false, false, false, 0);
-    Tmp1 = DAG.getStore(Tmp1.getValue(1), dl, Tmp1, Node->getOperand(1),
-                        MachinePointerInfo(VD), false, false, 0);
-    Results.push_back(Tmp1);
+  case ISD::VACOPY:
+    Results.push_back(DAG.expandVACopy(Node));
     break;
-  }
   case ISD::EXTRACT_VECTOR_ELT:
     if (Node->getOperand(0).getValueType().getVectorNumElements() == 1)
       // This must be an access of the only element.  Return it.
       Tmp1 = DAG.getNode(ISD::BITCAST, dl, Node->getValueType(0),
                          Node->getOperand(0));
     else
       Tmp1 = ExpandExtractFromVectorThroughStack(SDValue(Node, 0));
     Results.push_back(Tmp1);

lib/CodeGen/SelectionDAG/SelectionDAG.cpp

SDValue SelectionDAG::getShiftAmountOperand(EVT LHSTy, SDValue Op) {
   EVT OpTy = Op.getValueType();
   EVT ShTy = TM.getTargetLowering()->getShiftAmountTy(LHSTy);
   if (OpTy == ShTy || OpTy.isVector()) return Op;
 
   ISD::NodeType Opcode = OpTy.bitsGT(ShTy) ?  ISD::TRUNCATE : ISD::ZERO_EXTEND;
   return getNode(Opcode, SDLoc(Op), ShTy, Op);
 }
 
+SDValue SelectionDAG::expandVAArg(SDNode *Node) {
+  SDLoc dl(Node);
+  const TargetLowering &TLI = getTargetLoweringInfo();
+  const Value *V = cast<SrcValueSDNode>(Node->getOperand(2))->getValue();
+  EVT VT = Node->getValueType(0);
+  SDValue Tmp1 = Node->getOperand(0);
+  SDValue Tmp2 = Node->getOperand(1);
+  unsigned Align = Node->getConstantOperandVal(3);
+
+  SDValue VAListLoad = getLoad(TLI.getPointerTy(), dl, Tmp1, Tmp2,
+                               MachinePointerInfo(V), false, false, false, 0);
+  SDValue VAList = VAListLoad;
+
+  if (Align > TLI.getMinStackArgumentAlignment()) {
+    assert(((Align & (Align-1)) == 0) && "Expected Align to be a power of 2");
+
+    VAList = getNode(ISD::ADD, dl, VAList.getValueType(), VAList,
+                     getConstant(Align - 1, VAList.getValueType()));
+
+    VAList = getNode(ISD::AND, dl, VAList.getValueType(), VAList,
+                     getConstant(-(int64_t)Align, VAList.getValueType()));
+  }
+
+  // Increment the pointer, VAList, to the next vaarg
+  Tmp1 = getNode(ISD::ADD, dl, VAList.getValueType(), VAList,
+                 getConstant(TLI.getDataLayout()->
+                    getTypeAllocSize(VT.getTypeForEVT(*getContext())),
+                             VAList.getValueType()));
+  // Store the incremented VAList to the legalized pointer
+  Tmp1 = getStore(VAListLoad.getValue(1), dl, Tmp1, Tmp2,
+                  MachinePointerInfo(V), false, false, 0);
+  // Load the actual argument out of the pointer VAList
+  return getLoad(VT, dl, Tmp1, VAList, MachinePointerInfo(),
+                 false, false, false, 0);
+}
+
+SDValue SelectionDAG::expandVACopy(SDNode *Node) {
+  SDLoc dl(Node);
+  const TargetLowering &TLI = getTargetLoweringInfo();
+  // This defaults to loading a pointer from the input and storing it to the
+  // output, returning the chain.
+  const Value *VD = cast<SrcValueSDNode>(Node->getOperand(3))->getValue();
+  const Value *VS = cast<SrcValueSDNode>(Node->getOperand(4))->getValue();
+  SDValue Tmp1 = getLoad(TLI.getPointerTy(), dl, Node->getOperand(0),
+                         Node->getOperand(2), MachinePointerInfo(VS),
+                         false, false, false, 0);
+  return getStore(Tmp1.getValue(1), dl, Tmp1, Node->getOperand(1),
+                  MachinePointerInfo(VD), false, false, 0);
+}
+
 /// CreateStackTemporary - Create a stack temporary, suitable for holding the
 /// specified value type.
 SDValue SelectionDAG::CreateStackTemporary(EVT VT, unsigned minAlign) {
   MachineFrameInfo *FrameInfo = getMachineFunction().getFrameInfo();
   unsigned ByteSize = VT.getStoreSize();
   Type *Ty = VT.getTypeForEVT(*getContext());
   const TargetLowering *TLI = TM.getTargetLowering();
   unsigned StackAlign =

lib/Target/X86/X86ISelLowering.cpp

 
 SDValue X86TargetLowering::LowerVASTART(SDValue Op, SelectionDAG &DAG) const {
   MachineFunction &MF = DAG.getMachineFunction();
   X86MachineFunctionInfo *FuncInfo = MF.getInfo<X86MachineFunctionInfo>();
 
   const Value *SV = cast<SrcValueSDNode>(Op.getOperand(2))->getValue();
   SDLoc DL(Op);
 
-  if (!Subtarget->is64Bit() || Subtarget->isTargetWin64()) {
+  if (!Subtarget->is64Bit() || Subtarget->isTargetWin64() ||
+      MF.getFunction()->getCallingConv() == CallingConv::X86_64_Win64) {
     // vastart just stores the address of the VarArgsFrameIndex slot into the
     // memory location argument.
     SDValue FR = DAG.getFrameIndex(FuncInfo->getVarArgsFrameIndex(),
                                    getPointerTy());
     return DAG.getStore(Op.getOperand(0), DL, FR, Op.getOperand(1),
                         MachinePointerInfo(SV), false, false, 0);
   }
 

 
 SDValue X86TargetLowering::LowerVAARG(SDValue Op, SelectionDAG &DAG) const {
   assert(Subtarget->is64Bit() &&
          "LowerVAARG only handles 64-bit va_arg!");
   assert((Subtarget->isTargetLinux() ||
           Subtarget->isTargetDarwin()) &&
           "Unhandled target in LowerVAARG");
   assert(Op.getNode()->getNumOperands() == 4);
+
+  if (DAG.getMachineFunction().getFunction()->getCallingConv() ==
+      CallingConv::X86_64_Win64)
+    // The Win64 ABI uses char* instead of a structure.
+    return DAG.expandVAArg(Op.getNode());
+
   SDValue Chain = Op.getOperand(0);
   SDValue SrcPtr = Op.getOperand(1);
   const Value *SV = cast<SrcValueSDNode>(Op.getOperand(2))->getValue();
   unsigned Align = Op.getConstantOperandVal(3);
   SDLoc dl(Op);
 
   EVT ArgVT = Op.getNode()->getValueType(0);
   Type *ArgTy = ArgVT.getTypeForEVT(*DAG.getContext());
return DAG.getLoad(ArgVT, dl,
                      Chain,
                      VAARG,
                      MachinePointerInfo(),
                      false, false, false, 0);
 }
 
 static SDValue LowerVACOPY(SDValue Op, const X86Subtarget *Subtarget,
                            SelectionDAG &DAG) {
+  if (DAG.getMachineFunction().getFunction()->getCallingConv() ==
+      CallingConv::X86_64_Win64)
+    return DAG.expandVACopy(Op.getNode());
   // X86-64 va_list is a struct { i32, i32, i8*, i8* }.
   assert(Subtarget->is64Bit() && "This code only handles 64-bit va_copy!");
   SDValue Chain = Op.getOperand(0);
   SDValue DstPtr = Op.getOperand(1);
   SDValue SrcPtr = Op.getOperand(2);
   const Value *DstSV = cast<SrcValueSDNode>(Op.getOperand(3))->getValue();
   const Value *SrcSV = cast<SrcValueSDNode>(Op.getOperand(4))->getValue();
   SDLoc DL(Op);

lib/Transforms/Instrumentation/MemorySanitizer.cpp

Value *getShadowPtrForVAArgument(Value *A, IRBuilder<> &IRB,
                                    int ArgOffset) {
     Value *Base = IRB.CreatePointerCast(MS.VAArgTLS, MS.IntptrTy);
     Base = IRB.CreateAdd(Base, ConstantInt::get(MS.IntptrTy, ArgOffset));
     return IRB.CreateIntToPtr(Base, PointerType::get(MSV.getShadowTy(A), 0),
                               "_msarg");
   }
 
   void visitVAStartInst(VAStartInst &I) {
+    if (I.getParent()->getParent()->getCallingConv() ==
+        CallingConv::X86_64_Win64)
+      return;
     IRBuilder<> IRB(&I);
     VAStartInstrumentationList.push_back(&I);
     Value *VAListTag = I.getArgOperand(0);
     Value *ShadowPtr = MSV.getShadowPtr(VAListTag, IRB.getInt8Ty(), IRB);
 
     // Unpoison the whole __va_list_tag.
     // FIXME: magic ABI constants.
     IRB.CreateMemSet(ShadowPtr, Constant::getNullValue(IRB.getInt8Ty()),
                      /* size */24, /* alignment */8, false);
   }
 
   void visitVACopyInst(VACopyInst &I) {
+    if (I.getParent()->getParent()->getCallingConv() ==
+        CallingConv::X86_64_Win64)
+      return;
     IRBuilder<> IRB(&I);
     Value *VAListTag = I.getArgOperand(0);
     Value *ShadowPtr = MSV.getShadowPtr(VAListTag, IRB.getInt8Ty(), IRB);
 
     // Unpoison the whole __va_list_tag.
     // FIXME: magic ABI constants.
     IRB.CreateMemSet(ShadowPtr, Constant::getNullValue(IRB.getInt8Ty()),
                      /* size */24, /* alignment */8, false);
   }

test/CodeGen/X86/x86-64-ms_abi-vararg.ll

+; RUN: llc < %s -mcpu=generic -mtriple=x86_64-pc-linux-gnu | FileCheck %s
+
+; Verify that the var arg parameters which are passed in registers are stored
+; in home stack slots allocated by the caller and that AP is correctly
+; calculated.
+define x86_64_win64cc void @average_va(i32 %count, ...) nounwind {
+entry:
+; CHECK: movq   %r9, 32(%rsp)
+; CHECK: movq   %r8, 24(%rsp)
+; CHECK: movq   %rdx, 16(%rsp)
+; CHECK: leaq   16(%rsp), %rax
+
+  %ap = alloca i8*, align 8                       ; <i8**> [#uses=1]
+  %ap1 = bitcast i8** %ap to i8*                  ; <i8*> [#uses=1]
+  call void @llvm.va_start(i8* %ap1)
+  ret void
+}
+
+declare void @llvm.va_start(i8*) nounwind
+declare void @llvm.va_copy(i8*, i8*) nounwind
+
+; CHECK-LABEL: f5:
+; CHECK: leaq 48(%rsp),
+define x86_64_win64cc i8* @f5(i64 %a0, i64 %a1, i64 %a2, i64 %a3, i64 %a4, ...) nounwind {
+entry:
+  %ap = alloca i8*, align 8
+  %ap1 = bitcast i8** %ap to i8*
+  call void @llvm.va_start(i8* %ap1)
+  ret i8* %ap1
+}
+
+; CHECK-LABEL: f4:
+; CHECK: leaq 40(%rsp),
+define x86_64_win64cc i8* @f4(i64 %a0, i64 %a1, i64 %a2, i64 %a3, ...) nounwind {
+entry:
+  %ap = alloca i8*, align 8
+  %ap1 = bitcast i8** %ap to i8*
+  call void @llvm.va_start(i8* %ap1)
+  ret i8* %ap1
+}
+
+; CHECK-LABEL: f3:
+; CHECK: leaq 32(%rsp),
+define x86_64_win64cc i8* @f3(i64 %a0, i64 %a1, i64 %a2, ...) nounwind {
+entry:
+  %ap = alloca i8*, align 8
+  %ap1 = bitcast i8** %ap to i8*
+  call void @llvm.va_start(i8* %ap1)
+  ret i8* %ap1
+}
+
+; WinX86_64 uses char* for va_list. Verify that the correct amount of bytes
+; are copied using va_copy.
+
+; CHECK-LABEL: copy1:
+; CHECK: leaq 16(%rsp), [[REG_copy1:%[a-z]+]]
+; CHECK: movq [[REG_copy1]], -8(%rsp)
+; CHECK: movq [[REG_copy1]], -16(%rsp)
+; CHECK: ret
+define x86_64_win64cc void @copy1(i64 %a0, ...) nounwind {
+entry:
+  %ap = alloca i8*, align 8
+  %cp = alloca i8*, align 8
+  %ap1 = bitcast i8** %ap to i8*
+  %cp1 = bitcast i8** %cp to i8*
+  call void @llvm.va_start(i8* %ap1)
+  call void @llvm.va_copy(i8* %cp1, i8* %ap1)
+  ret void
+}
+
+; CHECK-LABEL: copy4:
+; CHECK: leaq 40(%rsp), [[REG_copy4:%[a-z]+]]
+; CHECK: movq [[REG_copy4]], -8(%rsp)
+; CHECK: movq [[REG_copy4]], -16(%rsp)
+; CHECK: ret
+define x86_64_win64cc void @copy4(i64 %a0, i64 %a1, i64 %a2, i64 %a3, ...) nounwind {
+entry:
+  %ap = alloca i8*, align 8
+  %cp = alloca i8*, align 8
+  %ap1 = bitcast i8** %ap to i8*
+  %cp1 = bitcast i8** %cp to i8*
+  call void @llvm.va_start(i8* %ap1)
+  call void @llvm.va_copy(i8* %cp1, i8* %ap1)
+  ret void
+}
+
+; CHECK-LABEL: arg4:
+; va_start:
+; CHECK: leaq 40(%rsp), [[REG_arg4_1:%[a-z]+]]
+; CHECK: movq [[REG_arg4_1]], -8(%rsp)
+; va_arg:
+; CHECK: leaq 44(%rsp), [[REG_arg4_2:%[a-z]+]]
+; CHECK: movq [[REG_arg4_2]], -8(%rsp)
+; CHECK: movl 40(%rsp), %eax
+; CHECK: ret
+define x86_64_win64cc i32 @arg4(i64 %a0, i64 %a1, i64 %a2, i64 %a3, ...) nounwind {
+entry:
+  %ap = alloca i8*, align 8
+  %ap1 = bitcast i8** %ap to i8*
+  call void @llvm.va_start(i8* %ap1)
+  %tmp = va_arg i8** %ap, i32
+  ret i32 %tmp
+}