The attached patch (gcc.patch) implements the gcc portion of packed types. I have only tested this on a few examples, so I may be missing some type conversions somewhere. The gcc patch requires llvm_extra.patch, not included in the previous emails. Andrew -------------- next part -------------- A non-text attachment was scrubbed... Name: llvm_extra.patch Type: text/x-patch Size: 1677 bytes Desc: not available URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20061206/1c88dea4/attachment.bin> -------------- next part -------------- A non-text attachment was scrubbed... Name: gcc.patch Type: text/x-patch Size: 2714 bytes Desc: not available URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20061206/1c88dea4/attachment-0001.bin>
On Dec 6, 2006, at 1:58 PM, Andrew Lenharth wrote:> The attached patch (gcc.patch) implements the gcc portion of packed > types. I have only tested this on a few examples, so I may be missing > some type conversions somewhere. > > The gcc patch requires llvm_extra.patch, not included in the > previous emails. > > Andrew > <llvm_extra.patch>llvm_extra.patch is fine, it can go in with the rest.> <gcc.patch>This patch isn't sufficient for two reasons: 1) it will turn structs that don't need to be packed into packed llvm structs. 2) it won't turn some that should be packed llvm structs into packed structs (e.g. strange ABI considerations force a field to be unaligned, not an attribute at the source level). Here's an example of #1: extern struct { int x; int y; int z; } __attribute((packed)) foos;; This should just be {int,int,int}, not <{int,int,int}>. The approach I'd like to see is: StructTypeConversionInfo should have a bool field "Packed" that is initialized to false. As fields are added to StructTypeConversionInfo, there are three cases: 1. GCC wants the field to be at an offset equal to where LLVM would place it. Just add the field. 2. GCC wants to place the field at an offset further from where llvm would place it. Just add enough dummy fields to pad it over to where it needs to be. 3. GCC wants to place the field at an offset closer to where LLVM would place it. When this happens, the current struct is marked packed and the existing fields have 'spacers' put in between them if necessary. This ensures we only make things 'llvm packed' if needed, and should allow the logic to be significantly simplified. Seem reasonable? -Chris -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20061207/ba8080b0/attachment.html>
On 12/8/06, Chris Lattner <clattner at apple.com> wrote:> <gcc.patch> > > This patch isn't sufficient for two reasons: 1) it will turn structs that > don't need to be packed into packed llvm structs. 2) it won't turn some > that should be packed llvm structs into packed structs (e.g. strange ABI > considerations force a field to be unaligned, not an attribute at the source > level).Yea, I've also come across a case: struct { short x __attribute((packed)); int y __attribute((packed)); } a = {1, 2}, b = ... ; That my patch doesn't handle;> Seem reasonable?Yea, having tried the logic on larger codes (aka the linux kernel) I've found more packed idioms I wasn't handling. I'll see what I can do. Andrew
On 12/8/06, Chris Lattner <clattner at apple.com> wrote:> This patch isn't sufficient for two reasons: 1) it will turn structs that > don't need to be packed into packed llvm structs. 2) it won't turn some > that should be packed llvm structs into packed structs (e.g. strange ABI > considerations force a field to be unaligned, not an attribute at the source > level). > > Here's an example of #1: > extern struct { int x; int y; int z; } __attribute((packed)) foos;; > This should just be {int,int,int}, not <{int,int,int}>.no, consider struct foo { int x; int y; int z; } __attribute((packed)); struct {char c; struct foo f;} bar; sizeof(bar) == 13 if struct foo wasn't packed it's allignment would force a larger size for bar; Andrew