This is an archive of the discontinued LLVM Phabricator instance.

[X86] Fix bug in vectorcall calling convention
ClosedPublic

Authored by wxiao3 on Feb 2 2019, 6:37 PM.

Details

Summary

Original implementation can't correctly handle m256 and m512 types passed
by reference through stack. This patch fixes it.

Diff Detail

Repository
rL LLVM

Event Timeline

wxiao3 created this revision.Feb 2 2019, 6:37 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 2 2019, 6:37 PM
wxiao3 edited the summary of this revision. (Show Details)Feb 2 2019, 6:39 PM
wxiao3 edited the summary of this revision. (Show Details)Feb 2 2019, 6:42 PM
rnk added inline comments.Feb 4 2019, 2:12 PM
lib/Target/X86/X86CallingConv.cpp
165 ↗(On Diff #184924)

One way to deal with this would be to find MCRegisterInfo and ask if this is a sub/super register of XMM0 so we don't have to enumerate all the X,Y,Z cases and inevitable W someday in the future. But I'd accept this fix as is because I'm not sure how to get the register info data here.

wxiao3 updated this revision to Diff 187727.Feb 20 2019, 11:20 PM

I have updated the patch so that we don't have to enumerate all the X,Y,Z cases and inevitable W someday in the future. Is it ok for merge?

wxiao3 marked an inline comment as done.Feb 20 2019, 11:24 PM
rnk accepted this revision.Feb 21 2019, 1:15 PM

lgtm

lib/Target/X86/X86CallingConv.cpp
165 ↗(On Diff #184924)

Eh, now I feel this made the code less readable since we still have to say XMM4/5 twice. Anyway, I'm not that concerned.

This revision is now accepted and ready to land.Feb 21 2019, 1:15 PM
wxiao3 updated this revision to Diff 188323.Feb 26 2019, 1:26 AM
wxiao3 marked an inline comment as done.
wxiao3 added inline comments.
lib/Target/X86/X86CallingConv.cpp
165 ↗(On Diff #184924)

The XMM4/5 only appear once. Can we merge it now?

rnk added inline comments.Feb 26 2019, 11:35 AM
lib/Target/X86/X86CallingConv.cpp
165 ↗(On Diff #184924)

Sure, I already approved the patch, but it needs a committer to submit it. I see this is your first patch in phab, so I'll go ahead and do that. Thanks for finding regsOverlap!

This revision was automatically updated to reflect the committed changes.