Re: [C++0x] contiguous bitfields race implementation

[Richard Guenther <richard dot guenther at gmail dot com>]

On Fri, Jul 29, 2011 at 11:37 AM, Richard Guenther
<richard.guenther@gmail.com> wrote:
> On Fri, Jul 29, 2011 at 4:12 AM, Aldy Hernandez <aldyh@redhat.com> wrote:
>> On 07/28/11 06:40, Richard Guenther wrote:
>>
>>> Looking at the C++ memory model what you need is indeed simple enough
>>> to recover here. ?Still this loop does quadratic work for a struct with
>>> N bitfield members and a function which stores into all of them.
>>> And that with a big constant factor as you build a component-ref
>>> and even unshare trees (which isn't necessary here anyway). ?In fact
>>> you could easily manually keep track of bitpos when walking adjacent
>>> bitfield members. ?An initial call to get_inner_reference on
>>> TREE_OPERAND (exp, 0) would give you the starting position of the record.
>>>
>>> That would still be quadratic of course.
>>
>> Actually, we don't need to call get_inner_reference at all. ?It seems
>> DECL_FIELD_BIT_OFFSET has all the information we need.
>>
>> How about we simplify things further as in the attached patch?
>>
>> Tested on x86-64 Linux.
>>
>> OK for mainline?
>
> Well ... byte pieces of the offset can be in the tree offset
> (DECL_FIELD_OFFSET). ?Only up to DECL_OFFSET_ALIGN bits
> are tracked in DECL_FIELD_BIT_OFFSET (and DECL_FIELD_OFFSET
> can be a non-constant - at least for Ada, not sure about C++).
>
> But - can you please expand a bit on the desired semantics of
> get_bit_range? ?Especially, relative to what is *bitstart / *bitend
> supposed to be? ?Why do you pass in bitpos and bitsize - they
> seem to be used as local variables only. ?Why is the check for
> thread-local storage in this function and not in the caller (and
> what's the magic [0,0] bit-range relative to?)?
>
> The existing get_inner_reference calls give you a bitpos relative
> to the start of the containing object - but
>
> ? ? ?/* If this is the last element in the structure, include the padding
> ? ? ? ? at the end of structure. ?*/
> ? ? ?*bitend = TREE_INT_CST_LOW (TYPE_SIZE (record_type)) - 1;
>
> will set *bitend to the size of the direct parent structure size, not the
> size of the underlying object. ?Your proposed patch changes
> bitpos to be relative to the direct parent structure.

Using TYPE_SIZE can also run into issues with C++ tail packing,
you need to use DECL_SIZE of the respective field instead.  Consider

struct A {
  int : 17;
};
struct B : public A {
  char c;
};

where I'm not sure we are not allowed to pack c into the tail padding
in A.  Also neither TYPE_SIZE nor DECL_SIZE have to be constant,
at least in Ada you can have a variable-sized array before, and in
C you can have a trailing one.

Richard.

> So - I guess you need to play with some testcases like
>
> struct {
> ? int some_padding;
> ? struct {
> ? ? ?int bitfield :1;
> ? } x;
> };
>
> and split / clarify some of get_bit_range comments.
>
> Thanks,
> Richard.
>
>>
>