On Wed, Jul 27, 2011 at 5:03 PM, Richard Guenther
<richard.guent...@gmail.com> wrote:
> On Wed, Jul 27, 2011 at 4:56 PM, Richard Guenther
> <richard.guent...@gmail.com> wrote:
>> On Wed, Jul 27, 2011 at 4:52 PM, Richard Guenther
>> <richard.guent...@gmail.com> wrote:
>>> On Tue, Jul 26, 2011 at 7:38 PM, Jason Merrill <ja...@redhat.com> wrote:
>>>> On 07/26/2011 10:32 AM, Aldy Hernandez wrote:
>>>>>
>>>>>> I think the adjustment above is intended to match the adjustment of the
>>>>>> address by bitregion_start/BITS_PER_UNIT, but the above seems to assume
>>>>>> that bitregion_start%BITS_PER_UNIT == 0.
>>>>>
>>>>> That was intentional. bitregion_start always falls on a byte boundary,
>>>>> does it not?
>>>>
>>>> Ah, yes, of course, it's bitnum that might not. The code changes look
>>>> good,
>>>> then.
>>>
>>> Looks like this was an approval ...
>>>
>>> Anyway, I don't think a --param is appropriate to control a flag whether
>>> to allow store data-races to be created. Why not use a regular option
>>> instead?
>>>
>>> I believe that any after-the-fact attempt to recover bitfield boundaries is
>>> going to fail unless you preserve more information during bitfield layout.
>>>
>>> Consider
>>>
>>> struct {
>>> char : 8;
>>> char : 0;
>>> char : 8;
>>> };
>>>
>>> where the : 0 isn't preserved in any way and you can't distinguish
>>> it from struct { char : 8; char : 8; }.
>>
>> Oh, and
>>
>> INNERDECL is the actual object being referenced.
>>
>> || (!ptr_deref_may_alias_global_p (innerdecl)
>>
>> is surely not what you want. That asks if *innerdecl is global memory.
>> I suppose you want is_global_var (innerdecl)? But with
>>
>> && (DECL_THREAD_LOCAL_P (innerdecl)
>> || !TREE_STATIC (innerdecl))))
>>
>> you can simply skip this test. Or what was it supposed to do?
>
> And
>
> t = build3 (COMPONENT_REF, TREE_TYPE (exp),
> unshare_expr (TREE_OPERAND (exp, 0)),
> fld, NULL_TREE);
> get_inner_reference (t, &bitsize, &bitpos, &offset,
> &mode, &unsignedp, &volatilep, true);
>
> for each field of a struct type is of course ... gross! In fact you already
> have the FIELD_DECL in the single caller! Yes I know there is not
> enough information preserved by bitfield layout - see my previous reply.
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.
For bitfield lowering I'd like to preserve a way to get from a field-decl to
the first field-decl of a group of bitfield members that occupy an aligned
amount of storage (as place_field assigns it). That wouldn't necessarily
match the first bitfield field in the C++ bitfield group sense but would
probably be sensible enough for conforming accesses (and you'd only
need to search forward from that first field looking for a zero-size
field). Now, the question is of course what to do for DECL_PACKED
fields (I suppose, simply ignore the C++ memory model as C++ doesn't
have a notion of packed or specially (mis-)aligned structs or bitfields).
Richard.
