Page MenuHomePhabricator

[X86] Fix handling of i128<->fp on Windows
ClosedPublic

Authored by mstorsjo on Sep 24 2021, 6:28 AM.

Details

Summary

On Windows, i128 arguments are passed as indirect arguments, and
they are returned in xmm0.

This is mostly fixed up by WinX86_64ABIInfo::classify in Clang, making
the IR functions return v2i64 instead of i128, and making the arguments
indirect. However for cases where libcalls are generated in the target
lowering, the lowering uses the default x86_64 calling convention for
i128, where they are passed/returned as a register pair.

Add custom lowering logic, similar to the existing logic for i128
div/mod (added in 4a406d32e97b1748c4eed6674a2c1819b9cf98ea),
manually making the libcall (while overriding the return type to
v2i64 or passing the arguments as pointers to arguments on the stack).

X86CallingConv.td doesn't seem to handle i128 at all, otherwise
the windows specific behaviours would ideally be implemented as
overrides there, in generic code, handling these cases automatically.

This fixes https://bugs.llvm.org/show_bug.cgi?id=48940.

Diff Detail

Event Timeline

mstorsjo created this revision.Sep 24 2021, 6:28 AM
mstorsjo requested review of this revision.Sep 24 2021, 6:28 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 24 2021, 6:28 AM

Looks similar to my previous patch (rG4a406d32e97b1748c4eed6674a2c1819b9cf98ea), so this makes sense to me.

X86CallingConv.td doesn't seem to handle i128 at all

Looking at the commit history, it seems like I tried that, but that since i128 was removed by the legalizer, it was not feasible:
https://reviews.llvm.org/D1998#37285

Looks similar to my previous patch (rG4a406d32e97b1748c4eed6674a2c1819b9cf98ea), so this makes sense to me.

X86CallingConv.td doesn't seem to handle i128 at all

Looking at the commit history, it seems like I tried that, but that since i128 was removed by the legalizer, it was not feasible:
https://reviews.llvm.org/D1998#37285

Oh, thanks for that context! (I didn't find the corresponding phabricator review for that patch as it wasn't mentioned in the commit message.) That makes it even clearer that we indeed probably need to do it this way.

rnk accepted this revision.Tue, Sep 28, 12:45 PM

lgtm

I remember looking at this code and thinking that there must be a better way to do this, but I guess it would entail making i128 legal somehow. Anyway, I don't have a suggestion ready, and this seems like a reasonable extension of the existing approach.

This revision is now accepted and ready to land.Tue, Sep 28, 12:45 PM
This revision was landed with ongoing or failed builds.Wed, Sep 29, 3:06 AM
This revision was automatically updated to reflect the committed changes.