This is the mail archive of the mailing list for the GCC project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: reverse bitfield patch

On Wed, Jul 2, 2014 at 7:10 AM, DJ Delorie <> wrote:
> Revisiting an old thread, as I still want to get this feature in...
>> >> Why do you need to change varasm.c at all?  The hunks seem to be
>> >> completely separate of the attribute.
>> >
>> > Because static constructors have fields in the original order, not the
>> > reversed order.  Otherwise code like this is miscompiled:
>> Err - the struct also has fields in the original order - only the bit positions
>> of the fields are different because of the layouting option.
> The order of the field decls in the type (stor-layout.c) is not
> changed, only the bit position information.  The order here *can't* be
> changed, because the C language assumes that parameters, initializers,
> etc are presented in the same order as the original declaration,
> regardless of the target-specific layout.
> When the program includes an initializer:
>> > struct foo a = { 1, 2, 3 };
> The order of 1, 2, and 3 need to correspond to the order of the
> bitfields in 'a', so we can change neither the order of the bitfields
> in 'a' nor the order of constructor fields.
> However, when we stream the initializer out to the .S file, we need to
> pack the bitfields in the right sequence to generate the right bit
> patterns in the final output image.  The code in varasm.c exists to
> make sure that the initializers for bitfields are written/packed in
> the correct order, to correspond to the bitfield positions.  I.e.  the
> 1,2,3 initializer needs to be written to the .S file as either 0x0123
> or 0x3210 depending on the bit positions.

Ok, but as we are dealing exclusively with bitfields there is
already output_constructor_bitfield which uses an intermediate
state to "pack" bits into units that are then emitted.  It shouldn't
be hard to change that to make it pack into the appropriate bits

> In neither case do we change the order of the fields in the type
> itself, i.e. the array/chain order.
>> And you expect no other code looks at fields of a structure and its
>> initializer?  It's bad to keep this not in-sync.  Thus I don't think it's
>> viable to re-order fields just because bit allocation is reversed.
> The fields are in sync.  The varasm.c change sorts the elements as
> they're being output into the byte stream in the .S, it doesn't sort
> the field definitions themselves.
>> > +         /* If the bitfield-order attribute has been used on this
>> > +            structure, the fields might not be in bit-order.  In that
>> > +            case, we need a separate representative for each
>> > +            field.  */
>> > The typical use-case for this feature is memory-mapped hardware, where
>> > pessimum access is preferred anyway.
>> I doubt that, looking at constraints for strict volatile bitfields.
> The code that handles representatives requires (via an assert, IIRC)
> that the bit offsets within a representative be in ascending order.

Well, because that's supposed to be an invariant in all record field-decls ...
(which you break - for example fold_ctor_reference and friends might
be unhappy about this as well).

Note that code expects that representatives are byte-aligned so better
would be to not assign representatives or make the code work with
the swapped layout (I see no reason why that shouldn't work - maybe
it works doing before swapping the layout)?

> I.e. gcc ICEs if I don't bypass this.  In the case of volatile
> bitfields, which would be the typical use case for a reversed
> bitfield, the access mode is going to match the type size regardless,
> so performance is not changed by this patch.

representatives are not about performance but about correctness.

I'm still not happy about the idea in general (why is this a bitfield
exclusive thing?  If a piece of HW is big/little-endian then even
regular fields would have that property.  Your patch comes with
no testcase - testcases should cover all attribute variants, multiple
bitfield (group) sizes and mixed initializations / reads / writes and
be best execute testcases.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]