On Fri, Aug 10, 2012 at 2:41 AM, Stefan Kristiansson <[email protected]> wrote: > On Fri, Aug 10, 2012 at 1:49 AM, Eli Friedman <[email protected]> wrote: >> On Wed, Aug 8, 2012 at 11:52 PM, Stefan Kristiansson >> <[email protected]> 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 _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
