[cfe-commits] r101178 - in /cfe/trunk/lib/CodeGen: CGObjCMac.cpp CGRecordLayout.h CGRecordLayoutBuilder.cpp

[Daniel Dunbar]

Hey Chris,

Thanks for the comments.

On Tue, Apr 13, 2010 at 9:12 PM, Chris Lattner <clattner at apple.com> wrote:
>
> On Apr 13, 2010, at 1:58 PM, Daniel Dunbar wrote:
>
>> Author: ddunbar
>> Date: Tue Apr 13 15:58:55 2010
>> New Revision: 101178
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=101178&view=rev
>> Log:
>> IRgen: Enhance CGBitFieldInfo with enough information to fully describe the "policy" with which a bit-field should be accessed.
>> - For now, these policies are computed to match the current IRgen strategy, although the new information isn't being used yet (except in -fdump-record-layouts).
>>
>> - Design comments appreciated.
>
> Nice!
>
>> +++ cfe/trunk/lib/CodeGen/CGObjCMac.cpp Tue Apr 13 15:58:55 2010
>> @@ -129,6 +129,17 @@
>>     new (CGF.CGM.getContext()) CGBitFieldInfo(
>>       LTy, FieldNo, BitOffset, BitFieldSize, IvarTy->isSignedIntegerType());
>>
>> +  // We always construct a single, possibly unaligned, access for this case.
>> +  Info->setNumComponents(1);
>> +  CGBitFieldInfo::AccessInfo &AI = Info->getComponent(0);
>> +  AI.FieldIndex = 0;
>> +  AI.FieldByteOffset = 0;
>> +  AI.FieldBitStart = BitOffset;
>> +  AI.AccessWidth = CGF.CGM.getContext().getTypeSize(IvarTy);
>> +  AI.AccessAlignment = 0;
>> +  AI.TargetBitOffset = 0;
>> +  AI.TargetBitWidth = BitFieldSize;
>
> This idiom seems really gross to me (using "get" to provide a reference that is then filled in) why not use a "set" or "add" interface?  Something like:

Yeah, I agree just haven't fixed yet. These will be constructed first,
then passed in to the CGBitFieldInfo which will then be an immutable
object. I'm waiting to kill off the other (old style) constructor
variables).

> CGBitFieldInfo::AccessInfo AI;
> AI.FieldIndex = 0;
> AI.FieldByteOffset = 0;
> AI.FieldBitStart = BitOffset;
> AI.AccessWidth = CGF.CGM.getContext().getTypeSize(IvarTy);
> AI.AccessAlignment = 0;
> AI.TargetBitOffset = 0;
> AI.TargetBitWidth = BitFieldSize;
> Info->addComponent(AI);
> ...
>
>>
>> +  struct AccessInfo {
>> +    /// Offset of the field to load in the LLVM structure, if any.
>> +    unsigned FieldIndex;
>> +
>
>> +    /// Byte offset from the field address, if any. This should generally be
>> +    /// unused as the cleanest IR comes from having a well-constructed LLVM type
>> +    /// with proper GEP instructions, but sometimes its use is required, for
>> +    /// example if an access is intended to straddle an LLVM field boundary.
>> +    unsigned FieldByteOffset;
>> +
>> +    /// Bit offset in the accessed value to use. The width is implied by \see
>> +    /// TargetBitWidth.
>> +    unsigned FieldBitStart;
>> +
>> +    /// Bit width of the memory access to perform.
>> +    unsigned AccessWidth;
>> +
>> +    /// The alignment of the memory access, or 0 if the default alignment should
>> +    /// be used.
>> +    //
>> +    // FIXME: Remove use of 0 to encode default, instead have IRgen do the right
>> +    // thing when it generates the code, if avoiding align directives is
>> +    // desired.
>> +    unsigned AccessAlignment;
>
>> +
>> +    /// Offset for the target value.
>> +    unsigned TargetBitOffset;
>> +
>> +    /// Number of bits in the access that are destined for the bit-field.
>> +    unsigned TargetBitWidth;
>
> These 6 fields can problem be "unsigned char"?

FieldByteOffset can't necessarily, but the others can be smaller yeah.
I'll take a look. OTOH, these objects are relatively rare since they
only show up for structures we actually access, so it's barely even
worth it.

 - Daniel

> Otherwise, looks great!
>
> -Chris
>
>