[PATCH] Remove MISALIGNED_INDIRECT_REF

[Richard Guenther]

On Wed, 29 Sep 2010, Ulrich Weigand wrote:

> Richard Guenther wrote:
> 
> > *************** expand_expr_real_1 (tree exp, rtx target
> > *** 8734,8739 ****
> > --- 8736,8760 ----
> >   	set_mem_addr_space (temp, as);
> >   	if (TREE_THIS_VOLATILE (exp))
> >   	  MEM_VOLATILE_P (temp) = 1;
> > + 	if (mode != BLKmode
> > + 	    && (unsigned) align < GET_MODE_ALIGNMENT (mode)
> > + 	    /* If the target does not have special handling for unaligned
> > + 	       loads of mode then it can use regular moves for them.  */
> > + 	    && ((icode = optab_handler (movmisalign_optab, mode))
> > + 		!= CODE_FOR_nothing))
> > + 	  {
> > + 	    rtx reg, insn;
> > + 
> > + 	    /* We've already validated the memory, and we're creating a
> > + 	       new pseudo destination.  The predicates really can't fail.  */
> > + 	    reg = gen_reg_rtx (mode);
> > + 
> > + 	    /* Nor can the insn generator.  */
> > + 	    insn = GEN_FCN (icode) (reg, temp);
> > + 	    emit_insn (insn);
> > + 
> > + 	    return reg;
> > + 	  }
> 
> In experimenting with a STRICT_ALIGNMENT target, I noticed that
> the MEM rtx passed to the movmisalign expander sometimes has the
> default mode alignment, even though this is not true (that's why
> we need to use movmisalign in the first place).
> 
> This is caused by this sequence:
> 
>         temp = gen_rtx_MEM (mode, op0);
>         set_mem_attributes (temp, exp, 0);
> 
> The naked MEM has no MEM_ATTRS structure.  According to the default
> rules for STRICT_ALIGNMENT, this implies that MEM_ALIGN returns the
> mode alignment:
> 
> /* For a MEM rtx, the alignment in bits.  We can use the alignment of the
>    mode as a default when STRICT_ALIGNMENT, but not if not.  */
> #define MEM_ALIGN(RTX)                                                  \
> (MEM_ATTRS (RTX) != 0 ? MEM_ATTRS (RTX)->align                          \
>  : (STRICT_ALIGNMENT && GET_MODE (RTX) != BLKmode                       \
>     ? GET_MODE_ALIGNMENT (GET_MODE (RTX)) : BITS_PER_UNIT))
> 
> When set_mem_attributes is called, this means that this is now used as the
> initial value of the local "align" variable:
> 
> void
> set_mem_attributes_minus_bitpos (rtx ref, tree t, int objectp,
>                                  HOST_WIDE_INT bitpos)
> {
> [...]
>   unsigned int align = MEM_ALIGN (ref);
> 
> and later on in this function, "align" seems to be only increased, never
> decremented, e.g. like here:
> 
>   else if (TREE_CODE (t) == MEM_REF)
>     {
> [snip]
>         /* ??? This isn't fully correct, we can't set the alignment from the
>            type in all cases.  */
>         align = MAX (align, TYPE_ALIGN (type));
> 
> so that initial (wrong) value remains ...
> 
> 
> This seems odd to me; in any case it results in an incorrect MEM_ALIGN value.
> 
> Should this be fixed in mainline (how?) even though I have no test there?

Yes, I noticed the above, too, and it is that way since forever.

The problem is that MEM_ALIGN defaults to mode-alignment only on
strict-alignment targets.  Something that can't be right, or at least
it has very odd consequences.  This was changed way way way in the
past by Kenner (IIRC) who "simplified" the logic, removing some
extra explicit alignment handling.

The only sane (as in conservatively correct) variant would be

#define MEM_ALIGN(RTX) (MEM_ATTRS (RTX) != 0 ? MEM_ATTRS (RTX)->align : 
BITS_PER_UNIT)

but I left it for strict-alignment target maintainers (or middle-end/rtl-
maintainers with some more "history") to sort out the existing issues.
Like for example

typedef int unaligned_int __attribute__((align(1)));

int __attribute__((noinline))
foo (unaligned_int *p)
{
  return *p;
}

int main()
{
  char c[5];
  return foo(&c[1]);
}

and watch it explode with all GCC versions on strict-align targets
(first of all because of the same problem as you noted, second of
course because the movmisaling handling is sth I added only for
4.6 and Jakub backported it to 4.5).

So, not "my" mess, really, but RMSs, Kenners and others' (if you
dare to try to go back in history ...).

My suggested fix (even if just applying that locally in
set_mem_attrs_minus_bitpos) would likely pessimize strict-align
targets, but you are free to give it some testing.

Richard.

-- 
Richard Guenther <rguenther@suse.de>
Novell / SUSE Labs
SUSE LINUX Products GmbH - Nuernberg - AG Nuernberg - HRB 16746 - GF: Markus Rex