Page MenuHomePhabricator

[AArch64] Fix windows vararg functions with floats in the fixed args
ClosedPublic

Authored by mstorsjo on Tue, Apr 13, 12:40 AM.

Details

Summary

On Windows, float arguments are normally passed in float registers
in the calling convention for regular functions. For variable
argument functions, floats are passed in integer registers. This
already was done correctly since many years.

However, the surprising bit was that floats among the fixed arguments
also are supposed to be passed in integer registers, contrary to regular
functions. (This also seems to be the behaviour on ARM though, both
on Windows, but also on e.g. hardfloat linux.)

In the calling convention, don't promote shorter floats to f64, but
convert them to integers of the same length. (Floats passed as part of
the actual variable arguments are promoted to double already on the
C/Clang level; the LLVM vararg calling convention doesn't do any
extra promotion of f32 to f64 - this matches how it works on X86 too.)

Technically, this is an ABI break compared to older LLVM versions,
but it fixes compatibility with the official platform ABI. (In practice,
floats among the fixed arguments in variable argument functions is
a pretty rare construct.)

Diff Detail

Event Timeline

mstorsjo created this revision.Tue, Apr 13, 12:40 AM
mstorsjo requested review of this revision.Tue, Apr 13, 12:40 AM
Herald added a project: Restricted Project. · View Herald TranscriptTue, Apr 13, 12:40 AM
rnk added inline comments.Tue, Apr 13, 1:04 PM
llvm/lib/Target/AArch64/GISel/AArch64CallLowering.cpp
561–562

Can we put the logic here?

mstorsjo added inline comments.Tue, Apr 13, 1:13 PM
llvm/lib/Target/AArch64/GISel/AArch64CallLowering.cpp
561–562

Hmm, that might be possible, I'll have a go at it.

mstorsjo added inline comments.Wed, Apr 14, 3:57 AM
llvm/lib/Target/AArch64/GISel/AArch64CallLowering.cpp
561–562

Actually, it looks to me like it'd be more of a mess than what this patch is; getAssignFnsForCC is called in 5 places, and we'd need to have access to the MachineFunction (for querying the subtarget, whether the calling convention is a windows one, and for querying whether the function is a variadic one). E.g. in the case of doCallerAndCalleePassArgsTheSameWay below, we have the MachineFunction for the caller, but for the callee, we only have a CallLoweringInfo struct which only contains a MachineOperand.

rnk added inline comments.Wed, Apr 14, 12:00 PM
llvm/lib/Target/AArch64/GISel/AArch64CallLowering.cpp
175

I'm guessing this would become UseVarArgsCCForFixed = IsWin && IsVarArg;

249

Really, I wanted to find a way to make this if more self-documenting.

261–262

Let's pre-calculate a boolean, something like UseVarArgsCCForFixed, and put that in the if above.

561–562

I think AArch64TargetLowering has the subtarget, but I see that it is private. You can use TLI to get the TargetMachine, which will tell you if we are on Windows, but it doesn't have access to the isCallingConvWin64 logic in AArch64Subtarget. That logic could be reimplemented inline here, but I think I agree, let's find another way.

mstorsjo updated this revision to Diff 337517.Wed, Apr 14, 12:20 PM

Adjusted AArch64CallLowering.cpp to use a UseVarArgsCCForFixed variable instead.

rnk accepted this revision.Wed, Apr 14, 6:35 PM

lgtm

This revision is now accepted and ready to land.Wed, Apr 14, 6:35 PM

@mstorsjo
Hi Martin,

Please consider backporting this and your other WoA fixes to release/12.x branch. They may or may not hit the upcoming LLVM 12.0.0 release, but, at least, they'll get into the subsequent update release.

@mstorsjo
Hi Martin,

Please consider backporting this and your other WoA fixes to release/12.x branch. They may or may not hit the upcoming LLVM 12.0.0 release, but, at least, they'll get into the subsequent update release.

@rnk - What do you think of backporting this kind of fix to 12.0.1 - is it too "big" a change (as it changes the ABI we produce, even if it was wrong before) for that part of the release process?

rnk added a comment.Thu, Apr 15, 1:01 PM

Please consider backporting this and your other WoA fixes to release/12.x branch. They may or may not hit the upcoming LLVM 12.0.0 release, but, at least, they'll get into the subsequent update release.

@rnk - What do you think of backporting this kind of fix to 12.0.1 - is it too "big" a change (as it changes the ABI we produce, even if it was wrong before) for that part of the release process?

I'd say go ahead and backport it. I think there are probably more MSVC/Clang WoA ABI boundaries than Clang <12.0.0/Clang 12.0.1 WoA ABI boundaries.

arsenm added a subscriber: arsenm.Tue, Apr 20, 6:11 AM
arsenm added inline comments.
llvm/lib/Target/AArch64/GISel/AArch64CallLowering.cpp
167

You don't need to add this parameter, you can check the CCState::isVarArg in the assignment function

mstorsjo added inline comments.Tue, Apr 20, 2:09 PM
llvm/lib/Target/AArch64/GISel/AArch64CallLowering.cpp
167

Ah, thanks! Patch upcoming.