Rename the enum value from X86_64_Win64 to plain Win64.
The symbol exposed in the textual IR is changed from 'x86_64_win64cc' to 'win64cc', but the numeric value is kept, keeping support for old bitcode.
Paths
| Differential D34474
[AArch64] Extend CallingConv::X86_64_Win64 to AArch64 as well ClosedPublic Authored by mstorsjo on Jun 21 2017, 1:25 PM.
Details Summary Rename the enum value from X86_64_Win64 to plain Win64. The symbol exposed in the textual IR is changed from 'x86_64_win64cc' to 'win64cc', but the numeric value is kept, keeping support for old bitcode.
Diff Detail
Event TimelineHerald added subscribers: kristof.beyls, javed.absar, mehdi_amini and 2 others. · View Herald TranscriptJun 21 2017, 1:25 PM mstorsjo added a child revision: D34475: [AArch64] Add support for __builtin_ms_va_list on aarch64.Jun 21 2017, 1:27 PM Comment Actions I don't think that we should be adding a new calling convention for this. Win/AArch64 is just AAPCS64. Just like ARM, the difference for the va_list can be abstracted out entirely in the frontend, even if it is through a new LLVM intrinsic. This revision now requires changes to proceed.Jun 27 2017, 1:12 PM Comment Actions I think MSVC has to have changed the ABI from AAPCS if they've managed to get a "void *" va_list to work; the extra complexity in AAPCS isn't there for the giggles. x86 appears to mirror floating args into integer registers. If code isn't expected to interoperate, this becomes analogous to the DarwinPCS calling convention (it exists within lib/Target/AArch64 and affects CodeGen but doesn't need to be exposed in IR). If the Wine use-case does involve interoperation (and is supported) then a separate IR-level calling convention is probably unavoidable. Comment Actions We are working on adding support for the Windows ARM64 COFF format and recently upstreamed two patches https://reviews.llvm.org/D34705 and https://reviews.llvm.org/D34706. --Mandeep Comment Actions
Indeed. Since I don't have access to the actual MSVC compiler for arm64, I can't easily verify what it does with float arguments to vararg functions (unless I can find a easily triggered printf case in any of the arm64 exes included in the win10 sdk), but the va_list is a plain void pointer at least.
Indeed. I can try to update the patch and clarify the comments that it is mostly-AAPCS compliant, but floats are passed in integer registers (which is a sensible hypothesis until someone can confirm or deny this). Mandeep, do you have access to proper MSVC tools for arm64? Could you verify how floats are passed as function arguments (in float registers or in integer registers) for both normal and variadic functions? Comment Actions @mstorsjo I have an MSVC compiler for ARM64 and I found that it always passes floats in integer registers: float a = 1.1; float b = 2.1; float c = 3.1; float d = 4.1; float e = 5.1; float f = 6.1; float g = 7.1; float h = 8.1; float i = 9.1; float j = 10.1; printf("%f %f %f %f %f %f %f %f %f %f", a, b, c, d, e, f, g, h, i, j); 0000000000000058: BD0033F0 str s16,[sp,#0x30] 000000000000005C: BD4033F0 ldr s16,[sp,#0x30] 0000000000000060: 1E22C219 fcvt d25,s16 0000000000000064: BD4037F0 ldr s16,[sp,#0x34] 0000000000000068: 1E22C218 fcvt d24,s16 000000000000006C: BD403BF0 ldr s16,[sp,#0x38] 0000000000000070: 1E22C217 fcvt d23,s16 0000000000000074: BD403FF0 ldr s16,[sp,#0x3C] 0000000000000078: 1E22C216 fcvt d22,s16 000000000000007C: BD4043F0 ldr s16,[sp,#0x40] 0000000000000080: 1E22C215 fcvt d21,s16 0000000000000084: BD4047F0 ldr s16,[sp,#0x44] 0000000000000088: 1E22C214 fcvt d20,s16 000000000000008C: BD404BF0 ldr s16,[sp,#0x48] 0000000000000090: 1E22C213 fcvt d19,s16 0000000000000094: BD404FF0 ldr s16,[sp,#0x4C] 0000000000000098: 1E22C212 fcvt d18,s16 000000000000009C: BD4053F0 ldr s16,[sp,#0x50] 00000000000000A0: 1E22C211 fcvt d17,s16 00000000000000A4: BD4057F0 ldr s16,[sp,#0x54] 00000000000000A8: 1E22C210 fcvt d16,s16 00000000000000AC: FD000BF9 str d25,[sp,#0x10] 00000000000000B0: FD0007F8 str d24,[sp,#8] 00000000000000B4: FD0003F7 str d23,[sp] 00000000000000B8: 9E6602C7 fmov x7,d22 00000000000000BC: 9E6602A6 fmov x6,d21 00000000000000C0: 9E660285 fmov x5,d20 00000000000000C4: 9E660264 fmov x4,d19 00000000000000C8: 9E660243 fmov x3,d18 00000000000000CC: 9E660222 fmov x2,d17 00000000000000D0: 9E660201 fmov x1,d16 00000000000000D4: 90000008 adrp x8,$SG5071 00000000000000D8: 91000100 add x0,x8,$SG5071 00000000000000DC: 94000000 bl printf Comment Actions foo(a, b, c, d, e, f, g, h, i, j); 000000000000012C: BD402BE7 ldr s7,[sp,#0x28] 0000000000000130: BD402FE6 ldr s6,[sp,#0x2C] 0000000000000134: BD4033E5 ldr s5,[sp,#0x30] 0000000000000138: BD4037E4 ldr s4,[sp,#0x34] 000000000000013C: BD403BE3 ldr s3,[sp,#0x38] 0000000000000140: BD403FE2 ldr s2,[sp,#0x3C] 0000000000000144: BD4043E1 ldr s1,[sp,#0x40] 0000000000000148: BD4047E0 ldr s0,[sp,#0x44] 000000000000014C: 94000000 bl foo Comment Actions
Great, thanks for checking! I've revised the patch now, by trying to store the variadic arguments directly below the rest of the arguments on the stack, so that they will work seamlessly as one single array. mstorsjo edited edge metadata. Comment ActionsUpdated to clarify that the windows arm64 calling convention actually differs from normal AAPCS when it comes to calling variadic functions. (Support for actually passing parameters that way is not yet implemented). I also updated it to try to store the variadic arguments directly below the rest of the arguments on the stack, so that they will work seamlessly as one single array. Comment Actions Ping - can someone who knows aarch64 and/or LLVM internals in general, comment on whether the stack handling is correct (I'm not confident about that part of code but just tried to look at the ARM and X86 targets to figure out enough to make it work). The generated function prologues start with two "sub sp, sp, #..." instructions - one for the area reserved for storing register arguments to fit in with the rest of the arguments on the stack, and one for the rest of the stack usage. Comment Actions I'm still not sure I understand why we need the different CC when X86_64 is able to handle both the SysV and the MS style VAArg lowering via the ABI::LowerVAArg switching between the two. I feel that we should follow a similar approach here. Comment Actions
This does in fact use the exat same approach as on x86_64. It's not an issue with LowerVAARG (where the handling of the MS style can be encapsulated on the clang level so there's no need for any support for that on the LLVM level at all), but the issue is with LowerVASTART. And for LowerVASTART, the handling is identical to the one on x86_64. The key parts from X86TargetLowering::LowerVASTART in lib/Target/X86/X86ISelLowering.cpp: SDValue X86TargetLowering::LowerVASTART(SDValue Op, SelectionDAG &DAG) const { if (!Subtarget.is64Bit() || Subtarget.isCallingConvWin64(MF.getFunction()->getCallingConv())) { And isCallingConvWin64 contains this (redacted): bool isCallingConvWin64(CallingConv::ID CC) const { switch (CC) { // On Win64, all these conventions just use the default convention. case CallingConv::C: return isTargetWin64(); // This convention allows using the Win64 convention on other targets. case CallingConv::X86_64_Win64: return true; // This convention allows using the SysV convention on Windows targets. case CallingConv::X86_64_SysV: return false; // Otherwise, who knows what this is. default: return false; } As long as the windows ABI only should be used while targeting windows, we indeed don't need a separate calling convention exposed in the IR. But in order to be able to implement wine, we need to be able to signal that this particular function uses a convention different from the default of the current target triplet. mstorsjo retitled this revision from [RFC] [AArch64] Add a win64 specific aarch64 calling convention, for va_list handling to [RFC] [AArch64] Add a win64 specific aarch64 calling convention for vararg functions. Comment ActionsImplemented correct handling of passing of floats in calls to vararg functions as well. Comment Actions I guess at this point, it'd make sense to split this into two patches, one for adding the general support for arm64/win varargs, and one for adding the extra keywords for enabling wine to use it in builds targeting Linux. I'll update the patches to that effect tomorrow. mstorsjo retitled this revision from [RFC] [AArch64] Add a win64 specific aarch64 calling convention for vararg functions to [RFC] [AArch64] Add a win64 specific aarch64 calling convention. mstorsjo added a parent revision: D35006: [AArch64] Implement support for windows style vararg functions. Comment ActionsSplit the patch into two, one prerequisite (D35006) which adds generic support for win64/arm64 varargs, and this one adds the necessary things for enabling this while targeting e.g. linux (basically for using it in wine). mstorsjo retitled this revision from [RFC] [AArch64] Add a win64 specific aarch64 calling convention to [AArch64] Add a win64 specific aarch64 calling convention. Comment ActionsAdded a test case, rebased on top of the version of D35006 that was committed, removed the RFC tag from the title. Comment Actions I commented on the clang side that I think it's worth having a single ms_abi calling convention, but I don't think it matters too much for LLVM. The only code that cares about the convention is going to be in lib/Target/{X86|AArch64} anyway. It might be worth a try, though. Comment Actions I agree with Reid. There are now 2 targets using this and it seems like a generic requirement of Windows & Wine interoperability; it's time to make it generic. Comment Actions
So, in unifying it and making it generic, what backwards compat requirements are there on the IR? Can I rename the calling convention enum (X86_64_Win64, to e.g. just Win64) and just force all callers (at least clang) to catch up at once? And what about textual IR? There are IR backwards compat tests that use the "x86_64_win64cc" name. Reid also hinted at wanting to hook up the "sysv_abi" attribute for the inverse generically, but for aarch64, there's not just one inverse but two (aapcs and darwin), so I'd rather only do the win64 one first (where there's a direct need) while the others only would be for completeness (afaik, at least so far). Comment Actions
I think so. As long as you don't change the enumerator number existing .bc files should be compatible, and they're only exposed as ints on the C interface so that's not an issue. It's fine to change the name in the backwards compatibility tests IMO. mstorsjo retitled this revision from [AArch64] Add a win64 specific aarch64 calling convention to [AArch64] Extend CallingConv::X86_64_Win64 to AArch64 as well. Comment ActionsUpdated as suggested, by changing the existing CallingConvention::X86_64_Win64 into a generic CallingConvention::Win64, retaining binary compatiblity with old bitcode but changing the textual representation. Comment Actions I think this looks pretty much as I'd expect. Thanks for working on the generic refactoring. Closed by commit rL308208: [AArch64] Extend CallingConv::X86_64_Win64 to AArch64 as well (authored by mstorsjo). · Explain WhyJul 17 2017, 1:06 PM This revision was automatically updated to reflect the committed changes.
Revision Contents
Diff 106924 llvm/trunk/include/llvm/IR/CallingConv.h
llvm/trunk/lib/AsmParser/LLLexer.cpp
llvm/trunk/lib/AsmParser/LLParser.cpp
llvm/trunk/lib/AsmParser/LLToken.h
llvm/trunk/lib/IR/AsmWriter.cpp
llvm/trunk/lib/Target/AArch64/AArch64FrameLowering.cpp
llvm/trunk/lib/Target/AArch64/AArch64ISelLowering.cpp
llvm/trunk/lib/Target/AArch64/AArch64Subtarget.h
llvm/trunk/lib/Target/X86/X86CallingConv.td
llvm/trunk/lib/Target/X86/X86FastISel.cpp
llvm/trunk/lib/Target/X86/X86ISelLowering.cpp
llvm/trunk/lib/Target/X86/X86RegisterInfo.cpp
llvm/trunk/lib/Target/X86/X86Subtarget.h
llvm/trunk/lib/Transforms/Instrumentation/MemorySanitizer.cpp
llvm/trunk/test/Bitcode/compatibility-3.6.ll
llvm/trunk/test/Bitcode/compatibility-3.7.ll
llvm/trunk/test/Bitcode/compatibility-3.8.ll
llvm/trunk/test/Bitcode/compatibility-3.9.ll
llvm/trunk/test/Bitcode/compatibility-4.0.ll
llvm/trunk/test/Bitcode/compatibility.ll
llvm/trunk/test/CodeGen/AArch64/aarch64_win64cc_vararg.ll
llvm/trunk/test/CodeGen/X86/2009-06-03-Win64DisableRedZone.ll
llvm/trunk/test/CodeGen/X86/fast-isel-x86-64.ll
llvm/trunk/test/CodeGen/X86/sibcall-win64.ll
llvm/trunk/test/CodeGen/X86/win64-nosse-csrs.ll
llvm/trunk/test/CodeGen/X86/win64_nonvol.ll
llvm/trunk/test/CodeGen/X86/win64_params.ll
llvm/trunk/test/CodeGen/X86/win_chkstk.ll
llvm/trunk/test/CodeGen/X86/win_coreclr_chkstk.ll
llvm/trunk/test/CodeGen/X86/x86-64-ms_abi-vararg.ll
llvm/trunk/utils/vim/syntax/llvm.vim
|