[cfe-commits] [PATCH] CodeGen support for transparent_union

[Eli Friedman]

On Fri, Aug 10, 2012 at 2:41 AM, Stefan Kristiansson
<stefan.kristiansson at saunalahti.fi> wrote:
> On Fri, Aug 10, 2012 at 1:49 AM, Eli Friedman <eli.friedman at gmail.com> wrote:
>> On Wed, Aug 8, 2012 at 11:52 PM, Stefan Kristiansson
>> <stefan.kristiansson at saunalahti.fi> wrote:
>>> Ping?
>>
>> Quick review:
>>
>
> Thanks a lot for taking the time to look at this!
>
>> The changes to lib/CodeGen/CodeGenFunction.h and
>> lib/CodeGen/CodeGenTypes.cpp don't make sense; transparent_union
>> doesn't affect the representation of values of the given type, just
>> the calling convention.  What issue are you trying to fix with those
>> changes?
>>
>
> To be honest, when looking at this again I can't come up with a good
> defense for those changes.
> Without them something like this:
>
> struct c {
>   int a[256];
> };
>
> typedef union {
>   struct c a;
> } ARG __attribute__ ((__transparent_union__));
>
> void foo(ARG u);
>
> void bar(struct c a)
> {
>   foo(a);
> }
>
> turns into:
> %struct.c = type { [256 x i32] }
> %union.ARG = type { %struct.c }
>
> define void @bar(%struct.c* nocapture byval align 8 %a) nounwind uwtable {
> entry:
>   %tmpcast = bitcast %struct.c* %a to %union.ARG*
>   call void @foo(%union.ARG* byval align 8 %tmpcast) nounwind
>   ret void
> }
>
> declare void @foo(%union.ARG* byval align 8)
>
> and with them into:
> %struct.c = type { [256 x i32] }
>
> define void @bar(%struct.c* nocapture byval align 8 %a) nounwind uwtable {
> entry:
>   call void @foo(%struct.c* byval align 8 %a) nounwind
>   ret void
> }
>
> declare void @foo(%struct.c* byval align 8)
>
>
> but this looks like an non-issue since ultimately the calling
> convention stays the same.
>
> Attached is an updated patch with the changes to
> lib/CodeGen/CodeGenFunction.h and lib/CodeGen/CodeGenTypes.cpp
> removed.
>
>> getFirstFieldInTransparentUnion is ugly, but transparent_union is
>> really an ABI thing, and there isn't any obvious alternative place to
>> handle that.
>>
>
> Ok, so I guess that's an necessary evil then.

It looks like the call to getFirstFieldInTransparentUnion isn't in the
right place on x86-32.

I'm suspicious that ABIArgInfo::getDirect() isn't going to do the
"right" thing in all cases (say, for example, a union with a "void*"
and a "double"); that said, it probably doesn't in practice, given the
expected usage of transparent_union.

-Eli