Having both FunctionABIInfo and ABIFunctionInfo is pretty confusing... do these need to be separate?

um, I've renamed it for the time being - if I feel it doesn't serve a purpose in a few days I'll put it somewhere else

Hmm... this is tricky. I'm not entirely sure what to do here. For the purpose of mapping ABIArgInfo, we should never encounter unions. For the struct case, things are tricky. Mostly, I believe we just want LLVM structs as a means of holding multiple values. E.g. a StructType for Direct arguments will normally get unpacked into individual arguments. In this case, we really don't want any kind of padding fillers in it. I think the only case that cares about that is CoerceAndExpand, which has two struct types, one just with the elements (which will be passed as separate arguments) and one that has the actual layout (with correct alignment etc).

This kind of makes me wonder whether it would make sense to start by storing llvm types in ABIArgInfo to match what clang currently does. abi::Type is conceptually nicer, but also not quite the right representation for everything (e.g. because it can't really represent the "struct without layout" case well).

we should never encounter unions

Oh yeah! My bad...

Also I'm assuming both times you referred to ABIArgInfo you were talking about abi::ABIArgInfo and not codegen::ABIArgInfo (should probably name that something slightly different sorry).

Given that assumption - I really don't think we need to bring llvm::Type into the library as the struct without layout case can be added to the ABI typesystem itself (add another type of abi::StructPacking or decide whether we need padding while converting). But agreed a lot of cleanup to be done here, I'll keep it in mind while writing the x86 target.

Also I'm assuming both times you referred to ABIArgInfo you were talking about abi::ABIArgInfo and not codegen::ABIArgInfo (should probably name that something slightly different sorry).

Yeah, I mean abi::ABIArgInfo. Btw, I wonder whether we should call these abi::ArgInfo etc, given that we already have abi in the namespace name...

Given that assumption - I really don't think we need to bring llvm::Type into the library as the struct without layout case can be added to the ABI typesystem itself (add another type of abi::StructPacking or decide whether we need padding while converting). But agreed a lot of cleanup to be done here, I'll keep it in mind while writing the x86 target.

Yeah, this is an option. I was mostly thinking about using llvm::Type as the path of least resistance to clang integration...

um, I've renamed it for the time being - if I feel it doesn't serve a purpose in a few days I'll put it somewhere else

we should never encounter unions

Oh yeah! My bad...

Also I'm assuming both times you referred to ABIArgInfo you were talking about abi::ABIArgInfo and not codegen::ABIArgInfo (should probably name that something slightly different sorry).

Given that assumption - I really don't think we need to bring llvm::Type into the library as the struct without layout case can be added to the ABI typesystem itself (add another type of abi::StructPacking or decide whether we need padding while converting). But agreed a lot of cleanup to be done here, I'll keep it in mind while writing the x86 target.

A note for the future: We probably need to do something to avoid leaking memory from these temporary types. We can probably checkpoint the type arena before computing ABIArgInfo and then restore the checkpoint once it's no longer needed.

Hm... do we really need this named member logic? Looking at CGRecordLowering::lowerUnion(), it seems like the named members are only relevant in the !isZeroInitializable case, but you don't actually handle that and instead always override with the type of a named member.

What happens if you just drop this named member logic and only keep the alignment/size based logic below?

Yeah, I didn't think ABI rules would care about ZeroInit, so it prefers named members and upgrades to a better type if it has better align/size. I think without it there was some ABI mismatch from clang.

The reason I'm asking is that I'd like to minimize the amount of "weird stuff" that the frontend has to pass in when creating types.

I think as implemented this logic is pretty weird and doesn't make a lot of sense -- I think it will only make a difference in the specific case where you first have an unnamed field with a "better" type, then a named field (with which we override) and then all fields (named or unnamed) with "worse" types. But not for example if we first have a named field and then a better type.

Hm, none of the tests fail if I drop the SeenNamedMember check, so I guess its fine :)

Just as a general note, in C++ ctors it's okay to use the same name for the ctor arg and the member. So something like DL(DL) works and is idiomatic.

Looking at this code, I doubt it does something correct for bit fields. Bit field layout is complicated.

For what kind of structs does ABITypeMapper actually get used? Does it only ever get used on "coerced structs" in practice, or also other ones?

Yeah, ABITypeMapper is used on everything.

Yeah was worried about Bitfield packing, but it didn't cause any blaring issues, so I assumed LLVM took care of it