Page MenuHomePhabricator

Allow sret on the second parameter as well as the first
ClosedPublic

Authored by rnk on May 5 2014, 5:47 PM.

Details

Summary

MSVC always places the implicit sret parameter after the implicit this
parameter of instance methods. We used to handle this for
x86_thiscallcc by allocating the sret parameter on the stack and leaving
the this pointer in ecx, but that doesn't handle alternative calling
conventions like cdecl, stdcall, fastcall, or the win64 convention.

Instead, change the verifier to allow sret on the second parameter.

This also requires changing the X86 backend to return the argument with
the sret parameter, instead of assuming that the sret parameter comes
first.

Diff Detail

Repository
rL LLVM

Event Timeline

rnk updated this revision to Diff 9091.May 5 2014, 5:47 PM
rnk retitled this revision from to Include intrin.h before windows.h as a workaround for the x64 self-host.
rnk updated this object.
rnk added reviewers: rafael.espindola, majnemer.
rnk added a subscriber: Unknown Object (MLST).
rnk added a comment.May 5 2014, 6:03 PM

Erp, this was two patches smashed together.

rnk updated this revision to Diff 9093.May 5 2014, 6:08 PM

remove silly patch

rnk retitled this revision from Include intrin.h before windows.h as a workaround for the x64 self-host to Allow sret on the second parameter as well as the first.May 5 2014, 6:08 PM
rnk updated this object.
rnk edited reviewers, added: rafael; removed: rafael.espindola.May 8 2014, 5:22 PM
majnemer accepted this revision.May 9 2014, 11:47 AM
majnemer edited edge metadata.

LGTM

lib/Target/X86/X86ISelLowering.cpp
2310 ↗(On Diff #9093)

We should assert I != E.

This revision is now accepted and ready to land.May 9 2014, 11:47 AM
rafael added inline comments.May 9 2014, 12:18 PM
lib/Target/X86/X86ISelLowering.cpp
2307 ↗(On Diff #9093)

Why the loop? If the rest of the code assumes that it is the first or second, it seems better to check only those too, or at least assert that I is 0 or 1.

rnk updated this revision to Diff 9275.May 9 2014, 2:23 PM
rnk edited edge metadata.
  • Fix issue where the first param is split and add tests
rnk added a comment.May 9 2014, 2:31 PM

I looked for other backends that return sret parameters and found that mips and sparc do this. Mips can be updated to handle sret on parameters other than the first, but sparc has more assumptions about sret, so I stuck in a report_fatal_error call.

lib/Target/X86/X86ISelLowering.cpp
2307 ↗(On Diff #9093)

I realized this loop was wrong anyway if codegen split the first parameter into multiple EVTs, like i64 to 2 i32s on a 32-bit platform. I moved it up into the loop producing InVals and added a test for it.

rnk closed this revision.May 19 2014, 7:23 AM
rnk updated this revision to Diff 9554.

Closed by commit rL208453 (authored by @rnk).