Re: [PATCH] Fix PR55882

[Richard Biener <rguenther at suse dot de>]

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.