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

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

test/CodeGen/X86/stdarg.ll

13

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

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–2

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

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);
+                       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((Subtarget->isTargetLinux() ||

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

test/CodeGen/X86/nosse-varargs.ll

 ; RUN: llc < %s -march=x86-64 -mattr=-sse | FileCheck %s -check-prefix=NOSSE
+; RUN: llc < %s -mtriple=x86_64-gnux32 -mattr=-sse | FileCheck %s -check-prefix=NOSSE
 ; RUN: llc < %s -march=x86-64 | FileCheck %s -check-prefix=YESSSE
+; RUN: llc < %s -mtriple=x86_64-gnux32 | FileCheck %s -check-prefix=YESSSE
 ; PR3403
 target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128"
 target triple = "x86_64-unknown-linux-gnu"
   %struct.__va_list_tag = type { i32, i32, i8*, i8* }
 
 ; NOSSE-NOT: xmm
 ; YESSSE: xmm
 define i32 @foo(float %a, i8* nocapture %fmt, ...) nounwind {
 entry:
   %ap = alloca [1 x %struct.__va_list_tag], align 8    ; <[1 x %struct.__va_list_tag]*> [#uses=4]

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]

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=X8664 -check-prefix=CHECK
+; RUN: llc < %s -mtriple=x86_64-linux-gnux32 | FileCheck %s -check-prefix=X32 -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.
+;
+; X8664-DAG: movq {{.*}}, 192(%rsp)
+; X8664-DAG: movq {{.*}}, 184(%rsp)
+; X8664-DAG: movl {{.*}}, 180(%rsp)
+; X8664-DAG: movl {{.*}}, 176(%rsp)
+; X32-DAG: movl {{.*}}, 196(%esp)
+; X32-DAG: movl {{.*}}, 192(%esp)
+; X32-DAG: movl {{.*}}, 188(%esp)
+; X32-DAG: movl {{.*}}, 184(%esp)
   %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