On Tue, Aug 30, 2011 at 8:13 PM, Aldy Hernandez <al...@redhat.com> wrote:
>
>> Btw, *byte_offset is still not relative to the containing object as
>> documented, but relative to the base object of the exp reference
>> tree (thus, to a in a.i.j.k.l instead of to a.i.j.k). If it were supposed
>> to be relative to a.i.j.k get_inner_reference would be not needed
>> either. Can you clarify what "containing object" means in the
>> overall comment please?
>
> I'm thoroughly confused here. Originally I had "inner decl", then we
> changed the nomenclature to "containing object", and now there's this
> "innermost reference".
Well, the nomenclature is not so important once the function only
computes one variant. Only because it doesn't right now I am
confused with the nomenclature trying to figure out what it is supposed
to be relative to ...
The containing object of a component-ref is TREE_OPERAND (exp, 0)
to me. The base object would be get_base_object (exp), which is
eventually what we want, right?
> What I mean to say is the "a" in a.i.j.k.l. How would you like me to call
> that? The innermost reference? The inner decl? Would this comment be
> acceptable:
>
> Given a COMPONENT_REF, this function calculates the byte offset
> from the innermost reference ("a" in a.i.j.k.l) to the start of the
> contiguous bit region containing the field in question.
from the base object ("a" in a.i.j.k.l) ...
would be fine with me.
>>
>> If it is really relative to the innermost reference of exp you can
>> "CSE" the offset of TREE_OPERAND (exp, 0) and do relative
>> adjustments for all the other get_inner_reference calls. For
>> example the
>>
>> + /* If we found the end of the bit field sequence, include the
>> + padding up to the next field... */
>> if (fld)
>> {
>> ...
>> + /* Calculate bitpos and offset of the next field. */
>> + get_inner_reference (build3 (COMPONENT_REF,
>> + TREE_TYPE (exp),
>> + TREE_OPERAND (exp, 0),
>> + fld, NULL_TREE),
>> + &tbitsize,&end_bitpos,&end_offset,
>> + &tmode,&tunsignedp,&tvolatilep, true);
>>
>> case is not correct anyway, fld may have variable position
>> (non-INTEGER_CST DECL_FIELD_OFFSET), you can't
>> assume
>
> Innermost here means "a" in a.i.j.k.l? If so, this is what we're currently
> doing, *byte_offset is the start of the bit region, and *bit_offset is the
> offset from that.
>
> First, I thought we couldn't get a variable position here because we are now
> handling that case at the beginning of the function with:
>
> /* Be as conservative as possible on variable offsets. */
> if (TREE_OPERAND (exp, 2)
> && !host_integerp (TREE_OPERAND (exp, 2), 1))
> {
> *byte_offset = TREE_OPERAND (exp, 2);
> *maxbits = BITS_PER_UNIT;
> *bit_offset = 0;
> return;
> }
>
> And even if we do get a variable position, I have so far being able to get
> away with this...
Did you test Ada and enable the C++ memory model? ;)
Btw, even if the bitfield we access (and thus the whole region) is at a
constant offset, the field _following_ the bitregion (the one you query
above with get_inner_reference) can be at variable offset. I suggest
to simply not include any padding in that case (which would be,
TREE_CODE (DECL_FIELD_OFFSET (fld)) != INTEGER_CST).
>>
>> + *maxbits = TREE_INT_CST_LOW (maxbits_tree);
>>
>> this thus.
>
> ...because the call to fold_build2 immediately preceding this will fold away
> the variable offset.
You hope so ;)
> Is what you want, that we call get_inner_reference once, and then use
> DECL_FIELD_OFFSET+DECL_FIELD_BIT_OFFSET to calculate any subsequent bit
> offset? I found this to be quite tricky with padding, and such, but am
> willing to give it a whirl again.
Yes.
> However, could I beg you to reconsider this, and get something working
> first, only later concentrating on removing the get_inner_reference() calls,
> and performing any other tweaks/optimizations?
Sure, it's fine to tweak this in a followup.
Thanks,
Richard.
> Aldy
>
