Re: [PATCH] Fix PR79256

[Richard Biener <rguenther at suse dot de>]

On Mon, 30 Jan 2017, Uros Bizjak wrote:

> On Mon, Jan 30, 2017 at 11:56 AM, Richard Biener <rguenther@suse.de> wrote:
> > On Mon, 30 Jan 2017, Jakub Jelinek wrote:
> >
> >> On Mon, Jan 30, 2017 at 11:47:51AM +0100, Richard Biener wrote:
> >> > On Mon, 30 Jan 2017, Uros Bizjak wrote:
> >> >
> >> > > > 2017-01-30  Richard Biener  <rguenther@suse.de>
> >> > > >
> >> > > > PR target/79277
> >> > > > * config/i386/i386-modes.def: Align DFmode properly.
> >> > >
> >> > > Index: gcc/config/i386/i386-modes.def
> >> > > ===================================================================
> >> > > --- gcc/config/i386/i386-modes.def (revision 245021)
> >> > > +++ gcc/config/i386/i386-modes.def (working copy)
> >> > > @@ -33,6 +33,7 @@ ADJUST_FLOAT_FORMAT (XF, (TARGET_128BIT_
> >> > >    : &ieee_extended_intel_96_format));
> >> > >  ADJUST_BYTESIZE  (XF, TARGET_128BIT_LONG_DOUBLE ? 16 : 12);
> >> > >  ADJUST_ALIGNMENT (XF, TARGET_128BIT_LONG_DOUBLE ? 16 : 4);
> >> > > +ADJUST_ALIGNMENT (DF, !TARGET_64BIT ? 4 : 8);
> >> > >
> >> > > Please avoid negative logic, just swap arms of the conditional around.
> >> >
> >> > It was just meant as an example fix.  I don't think this is appropriate
> >> > at this stage nor is it complete.  A full fix would basically make
> >> > x86_field_alignment unnecessary which limits most modes alignment
> >> > to 32bit (but not vector or 128bit float modes).  And the conditional
> >> > needs updating to honor TARGET_ALIGN_DOUBLE.
> >>
> >> Yeah, at least for GCC 7 that change (quite major ABI change) is too
> >> dangerous IMHO.
> >
> > Yes, __alignof__ (double) would change.  But I think for correctness
> > at least __alignof__ (*(double *)p) needs to change.  So another
> > way to fix this would be to change the FE to use a differently
> > aligned double type in contexts where the default one is wrong
> > (but we do have too many == double_type_node checks everywhere...).
> >
> > Btw, we should eventually change ADJUST_FIELD_ALIGN to take the
> > type of the field instead of the FIELD_DECL (as said, only frv.c
> > looks at the field, all others just look at its type).
> 
> Digging a bit more through the documentation:
> 
> `-malign-double'
> `-mno-align-double'
>      Control whether GCC aligns `double', `long double', and `long
>      long' variables on a two word boundary or a one word boundary.
>      Aligning `double' variables on a two word boundary will produce
>      code that runs somewhat faster on a `Pentium' at the expense of
>      more memory.
> 
>      On x86-64, `-malign-double' is enabled by default.
> 
>      *Warning:* if you use the `-malign-double' switch, structures
>      containing the above types will be aligned differently than the
>      published application binary interface specifications for the 386
>      and will not be binary compatible with structures in code compiled
>      without that switch.
> 
> The text above implies that on 32bit targets we already have alignment
> to a word boundary (4-byte), unless -malign-double is used. So,
> proposed i386-modes.def patch seems redundant to me.

double bar (double *p)
{
  return *p;
}

(insn 6 5 7 (set (reg:DF 90)
        (mem:DF (reg/v/f:SI 88 [ p ]) [1 *p_2(D)+0 S8 A64])) "t.c":3 -1
     (nil))

with -m32 and -mno-align-double.  The flag only affects alignment
of fields within structs.  So consider

struct X { int i; double x; } x;
foo (&x.x);

clearly the RTL for bar is bogus.

Richard.