On Mon, 30 Jan 2017, Uros Bizjak wrote:
> On Mon, Jan 30, 2017 at 11:56 AM, Richard Biener <rguent...@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 <rguent...@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.
