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

[Chris Lattner]

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:

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"?

Otherwise, looks great!

-Chris