On Wed, 9 Jan 2013, Eric Botcazou wrote:
> > This fixes PR55882 - set_mem_attributes_minus_bitpos misses to
> > account for the to-be applied bitpos when computing MEM_ALIGN.
> > It extracts alignment from 't' instead of &t - bitpos.
> >
> > Bootstrapped and tested on x86_64-unknown-linux-gnu, bootstrap
> > and regtest running on mips.
> >
> > Does this look sensible?
>
> I'm not sure, bitpos isn't taken into account in the other cases when the
> alignment is computed. adjust_address_1 is supposed to properly adjust the
> alignment by the time the offset is applied.
Yes, but alignment adjustment assumes the alignment is that of
the actual MEM, not that of the MEM_EXPR. Thus, MEM_ALIGN is the
alignment of MEM_EXPR - MEM_OFFSET.
It's true that the other paths setting align_computed have the
same issue. I can produce a followup (or preparatory patch)
that at least removes the code that is redundant. For example
/* If this is a decl, set the attributes of the MEM from it. */
if (DECL_P (t))
{
attrs.expr = t;
attrs.offset_known_p = true;
attrs.offset = 0;
apply_bitpos = bitpos;
new_size = DECL_SIZE_UNIT (t);
attrs.align = DECL_ALIGN (t);
align_computed = true;
}
the alignment computation can be handled by the get_object_alignment
path just fine, no need to duplicate it here, likewise for the
CONSTANT_CLASS_P case. The ARRAY_REF path is scary enough to
me (I'd rather drop it completely ... it's probably designed to
make more cases covered by nonoverlapping_component_refs_p?).
At least handling of DECL/COMPONENT_REF below the ARRAY_REFs
should be commonized - for example the COMPONENT_REF case
else if (TREE_CODE (t2) == COMPONENT_REF)
{
attrs.expr = t2;
attrs.offset_known_p = false;
if (host_integerp (off_tree, 1))
{
attrs.offset_known_p = true;
attrs.offset = tree_low_cst (off_tree, 1);
apply_bitpos = bitpos;
}
doesn't adjust 't' nor does it compute alignment. So it
clearly cannot be that MEM_ALIGN is supposed to be the
alignment of MEM_EXPR.
That is - the question boils down to _what_ should MEM_ALIGN
be the alignment of? In all the rest of the compiler it surely
is the alignment of XEXP (mem, 0) - to make it differ from that
during a brief period of expand_assignment sounds fragile at least.
I'll try to simplify code paths without changing behavior first.
Thanks,
Richard.
