On Fri, Jul 29, 2011 at 11:37 AM, Richard Guenther
<richard.guent...@gmail.com> wrote:
> On Fri, Jul 29, 2011 at 4:12 AM, Aldy Hernandez <al...@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.
>
>>
>
