[PATCH] D19756: Expand aggregate arguments more often on 32-bit Windows

[Reid Kleckner via cfe-commits]

rnk marked an inline comment as done.
rnk added a comment.

In http://reviews.llvm.org/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.


================
Comment at: lib/CodeGen/TargetInfo.cpp:1151
@@ +1150,3 @@
+    } else {
+      // Don't do this for dynamic classes.
+      if (CXXRD->isDynamicClass())
----------------
hans wrote:
> 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?
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; };

================
Comment at: lib/CodeGen/TargetInfo.cpp:1177
@@ +1176,3 @@
+
+  // We can do this if there was no alignment padding.
+  return Size == getContext().getTypeSize(Ty);
----------------
hans wrote:
> 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..
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.

================
Comment at: lib/CodeGen/TargetInfo.cpp:1397
@@ -1397,1 +1396,3 @@
                                              bool &NeedsPadding) const {
+  // On Windows, aggregates other than HFAs are never passed in registers, and
+  // they do not consume register slots. HFAs have already been dealt with at
----------------
hans wrote:
> Silly question: what's an HFA? (Also for the patch description)
"homogenous floating-point aggregate". I'll expand it here.

================
Comment at: test/CodeGen/windows-struct-abi.c:13
@@ -12,3 +12,3 @@
 
-// CHECK: define void @receive_f1(%struct.f1* byval align 4 %a0)
+// CHECK: define void @receive_f1(float %a0.0)
 
----------------
hans wrote:
> we're sure this float won't end up in a register right?
Yep


http://reviews.llvm.org/D19756