This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by jpp on Aug 25 2015, 4:56 PM.

Details

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.

Diff Detail

Event Timeline

jpp updated this revision to Diff 33153.Aug 25 2015, 4:56 PM
jpp retitled this revision from to x32. Fixes a bug in how struct va_list is initialized in x32.
jpp updated this object.
jpp added a reviewer: llvm-commits.
dschuff added a subscriber: dschuff.
dschuff added inline comments.
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 added a subscriber: hjl.tools.EditedAug 31 2015, 6:57 PM

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 edited edge metadata.Sep 2 2015, 1:18 PM

You missed:

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

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

dschuff edited subscribers, added: llvm-commits; removed: dschuff.
jpp added a comment.Sep 8 2015, 9:14 AM

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.

jpp updated this revision to Diff 34228.Sep 8 2015, 10:16 AM
jpp marked 2 inline comments as done.

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 updated this revision to Diff 34241.Sep 8 2015, 12:45 PM
jpp marked an inline comment as done.
jpp updated this revision to Diff 34243.Sep 8 2015, 12:52 PM

Some updates to the new test code.

dschuff added inline comments.Sep 8 2015, 12:53 PM
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 marked 2 inline comments as done.Sep 8 2015, 1:22 PM

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

jpp updated this revision to Diff 34244.Sep 8 2015, 1:23 PM

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

jpp updated this revision to Diff 34249.Sep 8 2015, 1:42 PM
jpp marked an inline comment as done.
dschuff accepted this revision.Sep 8 2015, 1:46 PM
dschuff edited edge metadata.
This revision is now accepted and ready to land.Sep 8 2015, 1:46 PM
This revision was automatically updated to reflect the committed changes.