[win64] x86-64 generates incorrect asm in function prolog/epilog trying to save XMM registers

[llvmbot]

Bugzilla Link	3739
Resolution	FIXED
Resolved on	Apr 03, 2017 10:11
Version	2.5
OS	Windows XP
Depends On	#3173
Reporter	LLVM Bugzilla Contributor
CC	@asl,@efriedma-quic,@sunfishcode,@rnk

Extended Description

On Win64, registers XMM6 - XMM15 are considered non-volatile and should be callee-saved.
The bug this bug is a clone of (#2801) attempted to fix this, but the patch suggested as the fix was only partially submitted; it added the XMM registers to X86RegisterInfo::getCalleeSavesRegs(), but that's all.

The X86 function prolog/epilog code does not know how to save/restore XMM registers and tries to emit a PUSH XMMn instruction, which is invalid; it emits bad assembly. (When jitting, it encodes a push of a GP register instead.)

For example:

target triple = "x86_64-pc-windows"

declare extern_weak i32 @​foo()

define i32 @​func() {
entry:
%r = call i32 @​foo() nounwind
ret i32 %r
}

% llvm-as < test.ll | llc -x86-asm-syntax=att
(I can't stand Intel asm syntax, so even though this is a Win64 target I try to lessen the pain)

_text segment 'DATA'
align 16
.globl _func
_func:
$label1:
pushq %xmm15
pushq %xmm14
pushq %xmm13
pushq %xmm12
pushq %xmm11
pushq %xmm10
pushq %xmm9
pushq %xmm8
pushq %xmm7
pushq %xmm6
pushq %rsi
pushq %rdi
subq $88, %rsp
call _foo
addq $88, %rsp
popq %rdi
popq %rsi
popq %xmm6
popq %xmm7
popq %xmm8
popq %xmm9
popq %xmm10
popq %xmm11
popq %xmm12
popq %xmm13
popq %xmm14
popq %xmm15
ret

---

Note also that not only does the prolog/epilog emit incorrect instructions for dealing with XMM*,
it also shouldn't be bothering to save/restore the registers in the first place since they are not used in the function. (In PEI::calculateCalleeSavedRegisters(), the Fn.getRegInfo().isPhysRegUsed(Reg) call always returns true for all the XMM registers if the Function being emitted has any call instructions.)

The part of the patch attached to #​2801 which was not applied did attempt to generate direct writes/reads to the stack for XMM registers instead of push/pop, but still doesn't ensure alignment correctly.

+++ This bug was initially created as a clone of Bug #​2801 +++

The x86-64 ABI specifies that XMM6 to XMM15 are non-volatile, and should be preserved by the callee as needed. It appears that LLVM currently doesn't do this, causing unpredictable behavior with floating-point calculations.

[efriedma-quic]

The reason XMM6-XMM15 are being saved is that X86Instr64bit.td claims that calls clobber those registers. If you want a local hack, you can just edit that. The simplest solution that would probably be acceptable to commit would be to add an isWin64 target feature, and make two variants of the "call" instruction depending on that feature.

You should try proposing the hack in attachment 2024 again to deal with the prolog/epilog code; it looks like it got lost in the attempt to make a clean patch.

I'm not sure what the issue with the alignment was with the previous version, but my best guess is that mixing generating the pushes and the stack stores is problematic. A two-pass solution would probably work, doing the integer registers the PUSH/POP way, then doing the XMM registers the fallback way.

[asl]

The reason XMM6-XMM15 are being saved is that X86Instr64bit.td claims that
calls clobber those registers. If you want a local hack, you can just edit
that. The simplest solution that would probably be acceptable to commit would
be to add an isWin64 target feature, and make two variants of the "call"
instruction depending on that feature.
Nice catch! The proper solution for this part of the problem, actually, would be to completely remove that defs list and make codegen to use information based on real ABI / CC, not some arbitrary 'default' case. It seems, that we already have 2 different ways to specify callee-saved registers, which is definitely hacky. :(

[asl]

Chris, this is not a win64 problem only. It will also touch any custom calling convention on x86-64, which is slightly different, than one, specified by AMD64 ABI (for Linux / Darwin, etc.).

[llvmbot]

Proposed patch
This is a proposed patch.

It includes the changes to X86InstrInfo::spillCalleeSavedRegisters from the cloned bug's path, with a further fix to not include the XMM register slots in the calleeSavedFrameSize calculation since we're not pushing those and so the stack pointer needs to adjust for them in emit prolog/epilog.

It creates new variants of the 64-bit CALL instructions in the X84Instr64bit.td which don't include the XMM regs or ESI/EDI in the DEFS and are used if Subtarget->IsTargetWin64().

It also disables the Red Zone for Win64, since that's SysV ABI-specific.

[llvmbot]

I think the above patch fixes the prolog/epilog problems for Win64 without affecting other targets. Please look it over and tell me if I did anything stupid.

I ran the dejavu test suite and didn't see any new failures in CodeGen/X86, and did some manual testing on Win64 to make sure XMM registers and ESI and EDI are saved only when used, and stack is aligned before XMM registers are written to it, and tail calls still work. I'll work on further tests to add to the test suite.

I took the approach suggested by Eli Friedman because I didn't feel comfortable with completely removing the DEFS from the 64-bit CALL instructions as suggested by Anton. I'll leave that to someone with more experience.

[asl]

Hi, Craig

This does not look like a proper solution. Actually, it even make the things worse! It's unacceptable to create bunch of new nodes / patters for each new calling convention. As I already mentioned, proper solution would include teaching codegen to instruct RegisterInfo for defs induced by non callee-saved registers, etc. totally eliminating redundant register list from .td (Eli correctly mentioned such solution as a "hack").

Redzone hunk is ok.

[llvmbot]

Well, I did what Eli said would be the minimal thing acceptable to commit, since I did add a Win64 target feature test as he said. Perhaps Eli can clarify if he meant something more than that. I was hoping this could be accepted as a temporary solution while a better long term one is worked on.

The patch fixes Win64 (which doesn't work at all currently) and I don't believe it breaks anything else. But obviously it's not ideal in that continues along a path that is not elegant or extensible in the face of future or user-defined calling conventions.

In any case, I think the part of the patch in X86InstrInfo::spillCalleeSavedRegisters() should be accepted, because it allows the XMM registers to be correctly spilled/restored instead of generating bad asm.
With this and the red zone fix, Win64 at least works, even though it still spills/restores more registers than it needs to.

I'll take another stab at doing it without cloning the call instructions for each CC, but I'm a newbie in the LLVM code base and will likely need some handholding. It seems like it would be reasonable to leave the registers which are always clobbered by a call regardless of calling conventions (e.g. EAX, FP1-7, EFLAGS) in the tablegen DEFS for the call instructions, and then programmatically add callee-saved registers (and possibly argument registers?) based on the CC.

It would be helpful if someone could give me a pointer as to where to begin with this, that is, where is the right place to manually add the Defs to the call instructions. Is X86TargetLowering::LowerCALL reasonable? Is that too early or late?

[efriedma-quic]

Well, I did what Eli said would be the minimal thing acceptable to commit,
since I did add a Win64 target feature test as he said. Perhaps Eli can
clarify if he meant something more than that. I was hoping this could be
accepted as a temporary solution while a better long term one is worked on.

That was precisely my line of thinking... but if Anton says it's not acceptable, I won't contradict him.

It would be helpful if someone could give me a pointer as to where to begin
with this, that is, where is the right place to manually add the Defs to the
call instructions. Is X86TargetLowering::LowerCALL reasonable? Is that too
early or late?

You could probably do something there, but it'd likely get shot down as a hack that's only slightly less ugly; it's really too early. The ideal solution is to make it part of TableGen along with the calling convention stuff and hook up the calculation of implicit definitions to it, but that's too complicated for anyone who isn't extremely familiar with the relevant code. So I'm not really sure what to do either.

[llvmbot]

I just noticed that the PowerPC code generator uses exactly the same technique I tried to use to fix the Win64 problem in order to distinguish between MachO and ELF ABI calls; there are forked variants of the BL, BLA, and BCTR instructions for each ABI calling convention.

So I submit again that my patch, or one like it, should be accepted as a stopgap to fix the bug before the necessary CodeGen calling convention rearchitecture is done to make it possible to do this in a more elegant way.

I fully agree that Anton's suggestion is the better long term way to go, but it requires a significant change that would affect the calling convention support for all targets, and I don't yet have the experience with LLVM or the time to pull that off, and no one else has stepped up to volunteer.

Is it really preferable to just leave Win64 completely broken? And reject a proposed patch that fixes the problem because it doesn't meet a high standard that wasn't similarly applied to the PPC target?

[efriedma-quic]

I just committed the X86RegisterInfo.cpp and X86InstrInfo.cpp bits in r72830 and r72836, since they seem uncontroversial. So at least we're generating correct code now :)

[efriedma-quic]

This appears to be fixed with trunk?

[asl]

This appears to be fixed with trunk?
Yes, but I'd consider the fix as a workaround in fact. As I noted it's not a good thing to create bunch of nodes just for every calling convention... I'm proposing to keep this PR around and close only when proper solution will be implemented.

[rnk]

I'm pretty sure this is fixed. We use normal "movaps" instructions to spill XMMs and emit .seh_savexmm CFI pseudos to do with them.