x32. Fixes a bug in how struct va_list is initialized in x32

[jpp]

Summary

This patch modifies X86TargetLowering::LowerVASTART so that struct va_list is initialized with 32 bit pointers in x32. It also enables all tests that call @llvm.va_start() for x32.

[dschuff]

lib/Target/X86/X86ISelLowering.cpp

15195 ↗

(On Diff #33153)

80 col wrap. maybe just clang-format the patch?

test/CodeGen/X86/stdarg.ll

13 ↗

(On Diff #33153)

80 col

[hjl.tools]

You missed:

static SDValue LowerVACOPY(SDValue Op, const X86Subtarget *Subtarget,
                           SelectionDAG &DAG) {
  // X86-64 va_list is a struct { i32, i32, i8*, i8* }, except on Windows,
  // where a va_list is still an i8*.
  assert(Subtarget->is64Bit() && "This code only handles 64-bit va_copy!");
  if (Subtarget->isCallingConvWin64(
        DAG.getMachineFunction().getFunction()->getCallingConv()))
    // Probably a Win64 va_copy.
    return DAG.expandVACopy(Op.getNode());

  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);

  return DAG.getMemcpy(Chain, DL, DstPtr, SrcPtr,
                       DAG.getIntPtrConstant(24, DL), 8, /*isVolatile*/false,
                                                            ^^^ It should be 16 for x32.
                       false, false,
                       MachinePointerInfo(DstSV), MachinePointerInfo(SrcSV));

[dschuff]

In D12346#236986, @hjl.tools wrote:

You missed:

static SDValue LowerVACOPY(SDValue Op, const X86Subtarget *Subtarget,
                           SelectionDAG &DAG) {

Good point. but we can probably make that a separate change.

[jpp]

In D12346#236986, @hjl.tools wrote:

You missed:

static SDValue LowerVACOPY(SDValue Op, const X86Subtarget *Subtarget,
                           SelectionDAG &DAG) {
  // X86-64 va_list is a struct { i32, i32, i8*, i8* }, except on Windows,
  // where a va_list is still an i8*.
  assert(Subtarget->is64Bit() && "This code only handles 64-bit va_copy!");
  if (Subtarget->isCallingConvWin64(
        DAG.getMachineFunction().getFunction()->getCallingConv()))
    // Probably a Win64 va_copy.
    return DAG.expandVACopy(Op.getNode());

  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);

  return DAG.getMemcpy(Chain, DL, DstPtr, SrcPtr,
                       DAG.getIntPtrConstant(24, DL), 8, /*isVolatile*/false,
                                                            ^^^ It should be 16 for x32.
                       false, false,
                       MachinePointerInfo(DstSV), MachinePointerInfo(SrcSV));

I am trying to get x32 to work one piece at a time, with small, self-contained patches. I don't mind fixing LowerVACOPY here, but I would rather do it in a separate CL, changing the appropriate tests. My ultimate goal is to have all/most x86 tests enable for x32. Let me know.

[hjl.tools]

I just wanted to point it out the related x32 issue. I opened those x32 bugs
which should be easy to fix:

https://llvm.org/bugs/show_bug.cgi?id=24706
https://llvm.org/bugs/show_bug.cgi?id=22762
https://llvm.org/bugs/show_bug.cgi?id=22676
https://llvm.org/bugs/show_bug.cgi?id=20441

[dschuff]

Just noticed one other thing

test/CodeGen/X86/nosse-varargs.ll

6 ↗

(On Diff #34228)

x32 has a different datalayout than x86-64, and there seems to be some 64-bit pointer assumptions baked into this bitcode. Maybe we should regenerate/modify this in a separate test for x32. (the same maybe applies to stdarg.ll although it looks like the gep operands are the only thing theres, so maybe that's OK)

[jpp]

Some updates to the new test code.

[dschuff]

test/CodeGen/X86/stdarg.ll

1 ↗

(On Diff #34241)

can we go back to one check-prefix now?

test/CodeGen/X86/x32-va_start.ll

1 ↗

(On Diff #34241)

I guess we could also do a NOSSE version in this file too?

[jpp]

In D12346#241558, @hjl.tools wrote:

I just wanted to point it out the related x32 issue. I opened those x32 bugs
which should be easy to fix:

https://llvm.org/bugs/show_bug.cgi?id=24706
https://llvm.org/bugs/show_bug.cgi?id=22762
https://llvm.org/bugs/show_bug.cgi?id=22676
https://llvm.org/bugs/show_bug.cgi?id=20441

Feel free to CC me on those bugs. I really want llvm's x32 to work, and I might take some of them.

[dschuff]

otherwise LGTM

test/CodeGen/X86/x32-va_start.ll

1 ↗

(On Diff #34244)

more common style is to remove the CHECK- from the SSE check prefixes, i.e.:

-check-prefix=CHECK -check-prefix=NOSSE

llvm/trunk/lib/Target/X86/X86ISelLowering.cpp

SDValue X86TargetLowering::LowerVASTART(SDValue Op, SelectionDAG &DAG) const {
   FIN = DAG.getNode(ISD::ADD, DL, PtrVT, FIN, DAG.getIntPtrConstant(4, DL));
   SDValue OVFIN = DAG.getFrameIndex(FuncInfo->getVarArgsFrameIndex(), PtrVT);
   Store = DAG.getStore(Op.getOperand(0), DL, OVFIN, FIN,
                        MachinePointerInfo(SV, 8),
                        false, false, 0);
   MemOps.push_back(Store);
 
   // Store ptr to reg_save_area.
-  FIN = DAG.getNode(ISD::ADD, DL, PtrVT, FIN, DAG.getIntPtrConstant(8, DL));
+  FIN = DAG.getNode(ISD::ADD, DL, PtrVT, FIN, DAG.getIntPtrConstant(
+      Subtarget->isTarget64BitLP64() ? 8 : 4, DL));
   SDValue RSFIN = DAG.getFrameIndex(FuncInfo->getRegSaveFrameIndex(), PtrVT);
-  Store = DAG.getStore(Op.getOperand(0), DL, RSFIN, FIN,
-                       MachinePointerInfo(SV, 16), false, false, 0);
+  Store = DAG.getStore(Op.getOperand(0), DL, RSFIN, FIN, MachinePointerInfo(
+      SV, Subtarget->isTarget64BitLP64() ? 16 : 12), false, false, 0);
   MemOps.push_back(Store);
   return DAG.getNode(ISD::TokenFactor, DL, MVT::Other, MemOps);
 }
 
 SDValue X86TargetLowering::LowerVAARG(SDValue Op, SelectionDAG &DAG) const {
   assert(Subtarget->is64Bit() &&
          "LowerVAARG only handles 64-bit va_arg!");
   assert(Op.getNode()->getNumOperands() == 4);

llvm/trunk/test/CodeGen/X86/musttail-varargs.ll

 ; RUN: llc < %s -enable-tail-merge=0 -mtriple=x86_64-linux | FileCheck %s --check-prefix=LINUX
+; RUN: llc < %s -enable-tail-merge=0 -mtriple=x86_64-linux-gnux32 | FileCheck %s --check-prefix=LINUX-X32
 ; RUN: llc < %s -enable-tail-merge=0 -mtriple=x86_64-windows | FileCheck %s --check-prefix=WINDOWS
 ; RUN: llc < %s -enable-tail-merge=0 -mtriple=i686-windows | FileCheck %s --check-prefix=X86
 
 ; Test that we actually spill and reload all arguments in the variadic argument
 ; pack. Doing a normal call will clobber all argument registers, and we will
 ; spill around it. A simple adjustment should not require any XMM spills.
 
 declare void @llvm.va_start(i8*) nounwind

 ; LINUX-DAG: movq {{.*}}, %rsi
 ; LINUX-DAG: movq {{.*}}, %rdx
 ; LINUX-DAG: movq {{.*}}, %rcx
 ; LINUX-DAG: movq {{.*}}, %r8
 ; LINUX-DAG: movq {{.*}}, %r9
 ; LINUX-DAG: movb {{.*}}, %al
 ; LINUX: jmpq *{{.*}}  # TAILCALL
 
+; LINUX-X32-LABEL: f_thunk:
+; LINUX-X32-DAG: movl %edi, {{.*}}
+; LINUX-X32-DAG: movq %rsi, {{.*}}
+; LINUX-X32-DAG: movq %rdx, {{.*}}
+; LINUX-X32-DAG: movq %rcx, {{.*}}
+; LINUX-X32-DAG: movq %r8, {{.*}}
+; LINUX-X32-DAG: movq %r9, {{.*}}
+; LINUX-X32-DAG: movb %al, {{.*}}
+; LINUX-X32-DAG: movaps %xmm0, {{[0-9]*}}(%esp)
+; LINUX-X32-DAG: movaps %xmm1, {{[0-9]*}}(%esp)
+; LINUX-X32-DAG: movaps %xmm2, {{[0-9]*}}(%esp)
+; LINUX-X32-DAG: movaps %xmm3, {{[0-9]*}}(%esp)
+; LINUX-X32-DAG: movaps %xmm4, {{[0-9]*}}(%esp)
+; LINUX-X32-DAG: movaps %xmm5, {{[0-9]*}}(%esp)
+; LINUX-X32-DAG: movaps %xmm6, {{[0-9]*}}(%esp)
+; LINUX-X32-DAG: movaps %xmm7, {{[0-9]*}}(%esp)
+; LINUX-X32: callq get_f
+; LINUX-X32-DAG: movaps {{[0-9]*}}(%esp), %xmm0
+; LINUX-X32-DAG: movaps {{[0-9]*}}(%esp), %xmm1
+; LINUX-X32-DAG: movaps {{[0-9]*}}(%esp), %xmm2
+; LINUX-X32-DAG: movaps {{[0-9]*}}(%esp), %xmm3
+; LINUX-X32-DAG: movaps {{[0-9]*}}(%esp), %xmm4
+; LINUX-X32-DAG: movaps {{[0-9]*}}(%esp), %xmm5
+; LINUX-X32-DAG: movaps {{[0-9]*}}(%esp), %xmm6
+; LINUX-X32-DAG: movaps {{[0-9]*}}(%esp), %xmm7
+; LINUX-X32-DAG: movl {{.*}}, %edi
+; LINUX-X32-DAG: movq {{.*}}, %rsi
+; LINUX-X32-DAG: movq {{.*}}, %rdx
+; LINUX-X32-DAG: movq {{.*}}, %rcx
+; LINUX-X32-DAG: movq {{.*}}, %r8
+; LINUX-X32-DAG: movq {{.*}}, %r9
+; LINUX-X32-DAG: movb {{.*}}, %al
+; LINUX-X32: jmpq *{{.*}}  # TAILCALL
+
 ; WINDOWS-LABEL: f_thunk:
 ; WINDOWS-NOT: mov{{.}}ps
 ; WINDOWS-DAG: movq %rdx, {{.*}}
 ; WINDOWS-DAG: movq %rcx, {{.*}}
 ; WINDOWS-DAG: movq %r8, {{.*}}
 ; WINDOWS-DAG: movq %r9, {{.*}}
 ; WINDOWS-NOT: mov{{.}}ps
 ; WINDOWS: callq get_f
define void @g_thunk(i8* %fptr_i8, ...) {
   musttail call void (i8*, ...) %fptr(i8* %fptr_i8, ...)
   ret void
 }
 
 ; LINUX-LABEL: g_thunk:
 ; LINUX-NOT: movq
 ; LINUX: jmpq *%rdi  # TAILCALL
 
+; LINUX-X32-LABEL: g_thunk:
+; LINUX-X32-DAG: movl %edi, %[[REG:e[abcd]x|ebp|esi|edi|r8|r9|r1[0-5]]]
+; LINUX-X32-DAG: jmpq *%[[REG]]  # TAILCALL
+
 ; WINDOWS-LABEL: g_thunk:
 ; WINDOWS-NOT: movq
 ; WINDOWS: jmpq *%rcx # TAILCALL
 
 ; X86-LABEL: _g_thunk:
 ; X86: jmpl *%eax # TAILCALL
 
 ; Do a simple multi-exit multi-bb test.
else:
   musttail call void (%struct.Foo*, ...) %b(%struct.Foo* %this, ...)
   ret void
 }
 
 ; LINUX-LABEL: h_thunk:
 ; LINUX: jne
 ; LINUX: jmpq *{{.*}} # TAILCALL
 ; LINUX: jmpq *{{.*}} # TAILCALL
+; LINUX-X32-LABEL: h_thunk:
+; LINUX-X32: jne
+; LINUX-X32: jmpq *{{.*}} # TAILCALL
+; LINUX-X32: jmpq *{{.*}} # TAILCALL
 ; WINDOWS-LABEL: h_thunk:
 ; WINDOWS: jne
 ; WINDOWS: jmpq *{{.*}} # TAILCALL
 ; WINDOWS: jmpq *{{.*}} # TAILCALL
 ; X86-LABEL: _h_thunk:
 ; X86: jne
 ; X86: jmpl *{{.*}} # TAILCALL
 ; X86: jmpl *{{.*}} # TAILCALL

llvm/trunk/test/CodeGen/X86/soft-fp.ll

 ; RUN: llc < %s -march=x86    -mattr=+sse2,+soft-float | FileCheck %s
 ; RUN: llc < %s -march=x86-64 -mattr=+sse2,+soft-float | FileCheck %s
+; RUN: llc < %s -mtriple=x86_64-gnux32 -mattr=+sse2,+soft-float | FileCheck %s
 
 ; CHECK-NOT: xmm{[0-9]+}
 
 %struct.__va_list_tag = type { i32, i32, i8*, i8* }
 
 define i32 @t1(i32 %a, ...) nounwind {
 entry:
   %va = alloca [1 x %struct.__va_list_tag], align 8    ; <[1 x %struct.__va_list_tag]*> [#uses=2]

llvm/trunk/test/CodeGen/X86/stdarg.ll

-; RUN: llc < %s -mtriple=x86_64-linux | FileCheck %s
-; CHECK: testb %al, %al
+; RUN: llc < %s -mtriple=x86_64-linux | FileCheck %s -check-prefix=CHECK
 
 %struct.__va_list_tag = type { i32, i32, i8*, i8* }
 
 define void @foo(i32 %x, ...) nounwind {
 entry:
   %ap = alloca [1 x %struct.__va_list_tag], align 8; <[1 x %struct.__va_list_tag]*> [#uses=2]
   %ap12 = bitcast [1 x %struct.__va_list_tag]* %ap to i8*; <i8*> [#uses=2]
   call void @llvm.va_start(i8* %ap12)
+; CHECK: testb %al, %al
+
+; These test for specific offsets, which is very fragile. Still, the test needs
+; to ensure that va_list is correctly handled by both x86-64 and x32.
+;
+; CHECK-DAG: movq {{.*}}, 192(%rsp)
+; CHECK-DAG: movq {{.*}}, 184(%rsp)
+; CHECK-DAG: movl {{.*}}, 180(%rsp)
+; CHECK-DAG: movl {{.*}}, 176(%rsp)
   %ap3 = getelementptr inbounds [1 x %struct.__va_list_tag], [1 x %struct.__va_list_tag]* %ap, i64 0, i64 0; <%struct.__va_list_tag*> [#uses=1]
   call void @bar(%struct.__va_list_tag* %ap3) nounwind
   call void @llvm.va_end(i8* %ap12)
   ret void
 }
 
 declare void @llvm.va_start(i8*) nounwind
 
 declare void @bar(%struct.__va_list_tag*)
 
 declare void @llvm.va_end(i8*) nounwind

llvm/trunk/test/CodeGen/X86/x32-va_start.ll

+; RUN: llc < %s -mtriple=x86_64-linux-gnux32 | FileCheck %s -check-prefix=CHECK -check-prefix=SSE
+; RUN: llc < %s -mtriple=x86_64-linux-gnux32 -mattr=-sse | FileCheck %s -check-prefix=CHECK -check-prefix=NOSSE
+;
+; Verifies that x32 va_start lowering is sane. To regenerate this test, use
+; cat <<EOF |
+; #include <stdarg.h>
+;
+; int foo(float a, const char* fmt, ...) {
+;   va_list ap;
+;   va_start(ap, fmt);
+;   int value = va_arg(ap, int);
+;   va_end(ap);
+;   return value;
+; }
+; EOF
+; build/bin/clang -mx32 -O3 -o- -S -emit-llvm -xc -
+;
+target datalayout = "e-m:e-p:32:32-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnux32"
+
+%struct.__va_list_tag = type { i32, i32, i8*, i8* }
+
+define i32 @foo(float %a, i8* nocapture readnone %fmt, ...) nounwind {
+entry:
+  %ap = alloca [1 x %struct.__va_list_tag], align 16
+  %0 = bitcast [1 x %struct.__va_list_tag]* %ap to i8*
+  call void @llvm.lifetime.start(i64 16, i8* %0) #2
+  call void @llvm.va_start(i8* %0)
+; SSE: subl $72, %esp
+; SSE: testb %al, %al
+; SSE: je .[[NOFP:.*]]
+; SSE-DAG: movaps %xmm1
+; SSE-DAG: movaps %xmm2
+; SSE-DAG: movaps %xmm3
+; SSE-DAG: movaps %xmm4
+; SSE-DAG: movaps %xmm5
+; SSE-DAG: movaps %xmm6
+; SSE-DAG: movaps %xmm7
+; NOSSE-NOT: xmm
+; SSE: .[[NOFP]]:
+; CHECK-DAG: movq %r9
+; CHECK-DAG: movq %r8
+; CHECK-DAG: movq %rcx
+; CHECK-DAG: movq %rdx
+; CHECK-DAG: movq %rsi
+  %gp_offset_p = getelementptr inbounds [1 x %struct.__va_list_tag], [1 x %struct.__va_list_tag]* %ap, i32 0, i32 0, i32 0
+  %gp_offset = load i32, i32* %gp_offset_p, align 16
+  %fits_in_gp = icmp ult i32 %gp_offset, 41
+  br i1 %fits_in_gp, label %vaarg.in_reg, label %vaarg.in_mem
+; CHECK: cmpl $40, [[COUNT:.*]]
+; CHECK: ja .[[IN_MEM:.*]]
+
+vaarg.in_reg:                                     ; preds = %entry
+  %1 = getelementptr inbounds [1 x %struct.__va_list_tag], [1 x %struct.__va_list_tag]* %ap, i32 0, i32 0, i32 3
+  %reg_save_area = load i8*, i8** %1, align 4
+  %2 = getelementptr i8, i8* %reg_save_area, i32 %gp_offset
+  %3 = add i32 %gp_offset, 8
+  store i32 %3, i32* %gp_offset_p, align 16
+  br label %vaarg.end
+; CHECK: movl {{[^,]*}}, [[ADDR:.*]]
+; CHECK: addl [[COUNT]], [[ADDR]]
+; SSE: jmp .[[END:.*]]
+; NOSSE: movl ([[ADDR]]), %eax
+; NOSSE: retq
+; CHECK: .[[IN_MEM]]:
+vaarg.in_mem:                                     ; preds = %entry
+  %overflow_arg_area_p = getelementptr inbounds [1 x %struct.__va_list_tag], [1 x %struct.__va_list_tag]* %ap, i32 0, i32 0, i32 2
+  %overflow_arg_area = load i8*, i8** %overflow_arg_area_p, align 8
+  %overflow_arg_area.next = getelementptr i8, i8* %overflow_arg_area, i32 8
+  store i8* %overflow_arg_area.next, i8** %overflow_arg_area_p, align 8
+  br label %vaarg.end
+; CHECK: movl {{[^,]*}}, [[ADDR]]
+; NOSSE: movl ([[ADDR]]), %eax
+; NOSSE: retq
+; SSE: .[[END]]:
+
+vaarg.end:                                        ; preds = %vaarg.in_mem, %vaarg.in_reg
+  %vaarg.addr.in = phi i8* [ %2, %vaarg.in_reg ], [ %overflow_arg_area, %vaarg.in_mem ]
+  %vaarg.addr = bitcast i8* %vaarg.addr.in to i32*
+  %4 = load i32, i32* %vaarg.addr, align 4
+  call void @llvm.va_end(i8* %0)
+  call void @llvm.lifetime.end(i64 16, i8* %0) #2
+  ret i32 %4
+; SSE: movl ([[ADDR]]), %eax
+; SSE: retq
+}
+
+; Function Attrs: nounwind argmemonly
+declare void @llvm.lifetime.start(i64, i8* nocapture) nounwind
+
+; Function Attrs: nounwind
+declare void @llvm.va_start(i8*) nounwind
+
+; Function Attrs: nounwind
+declare void @llvm.va_end(i8*) nounwind
+
+; Function Attrs: nounwind argmemonly
+declare void @llvm.lifetime.end(i64, i8* nocapture) nounwind
+