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

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

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.

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.

>