This is an archive of the discontinued LLVM Phabricator instance.

Expand aggregate arguments more often on 32-bit Windows
ClosedPublic

Authored by rnk on Apr 29 2016, 6:14 PM.

Details

Summary

Before this change, we would pass all non-HFA record arguments on
Windows with byval. Byval often blocks optimizations and results in bad
code generation. Windows now uses the existing workaround that other
x86_32 platforms use.

I also expanded the workaround handle C++ records with constructors on
Windows. On non-Windows platforms, we probably have to keep generating
the same LLVM IR prototypes if we want our bitcode to be ABI compatible.
Otherwise we will encounter mismatch issues like PR21573.

Diff Detail

Repository
rL LLVM

Event Timeline

rnk updated this revision to Diff 55695.Apr 29 2016, 6:14 PM
rnk retitled this revision from to Expand aggregate arguments more often on 32-bit Windows.
rnk updated this object.
rnk added reviewers: majnemer, hans.
rnk added a subscriber: cfe-commits.
hans accepted this revision.Apr 29 2016, 9:22 PM
hans edited edge metadata.

This is awesome! lgtm

I only have nits and questions.

Want to reference PR27522 in the patch description?

Also in the description:

I also expanded the workaround handle C++ records with constructors on

is missing a "to".

lib/CodeGen/TargetInfo.cpp
1134 ↗(On Diff #55695)

maybe drop the function name from the comment while here

1151 ↗(On Diff #55695)

Why not for dynamic classes or non-empty bases? Is it just that they hold more data than the immediate fields, or is there some other reason?

1177 ↗(On Diff #55695)

Could there be a situation where the struct is packed, but there would be padding when we expand it?

I suppose it won't happen in practice since we only allow 32/64-bit members..

1397 ↗(On Diff #55695)

Silly question: what's an HFA? (Also for the patch description)

test/CodeGen/windows-struct-abi.c
13 ↗(On Diff #55695)

we're sure this float won't end up in a register right?

This revision is now accepted and ready to land.Apr 29 2016, 9:22 PM
rnk marked an inline comment as done.May 2 2016, 10:42 AM
In D19756#417718, @hans wrote:

This is awesome! lgtm

Great!

Want to reference PR27522 in the patch description?

Also in the description:

I also expanded the workaround handle C++ records with constructors on

is missing a "to".

Done.

lib/CodeGen/TargetInfo.cpp
1151 ↗(On Diff #55695)

These are the two main cases that "isCLike" was probably defending against. Ultimately the sizeof check below would detect non-empty bases and dynamic classes as padding bytes that didn't come from fields, but I felt like we should try to be safe up front.

Eventually it might be worth making this algorithm work recursively so that this gets passed as two i32s:

struct A { int a; };
struct B : A { int b; };
1177 ↗(On Diff #55695)

Stack arguments usually only get 4 byte alignment regardless of type. The purpose of the 32/64 check is to detect when expanding arguments would create padding. Maybe I should write that down in a comment here.

1397 ↗(On Diff #55695)

"homogenous floating-point aggregate". I'll expand it here.

test/CodeGen/windows-struct-abi.c
13 ↗(On Diff #55695)

Yep

This revision was automatically updated to reflect the committed changes.