diff mbox

Fix PR79256

Message ID CAFULd4YHLfwZ1TGE2gdRdEEEiNXRdGuROJB6wHvNLWa0eH=K8A@mail.gmail.com
State New
Headers show

Commit Message

Uros Bizjak Jan. 30, 2017, 10:41 a.m. UTC 
> 2017-01-30  Richard Biener  <rguenther@suse.de>
>
> PR target/79277
> * config/i386/i386-modes.def: Align DFmode properly.

Uros.

Comments

Richard Biener Jan. 30, 2017, 10:47 a.m. UTC | #1 
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.

So I leave such change to target maintainers and continue papering
over this issue in the middle-end (at least for GCC 7).

Thanks,
Richard.
Jakub Jelinek Jan. 30, 2017, 10:52 a.m. UTC | #2 
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.

	Jakub
Richard Biener Jan. 30, 2017, 10:56 a.m. UTC | #3 
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).

Richard.
Uros Bizjak Jan. 30, 2017, 11:22 a.m. UTC | #4 
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.

Uros.
Richard Biener Jan. 30, 2017, 11:26 a.m. UTC | #5 
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.
diff mbox

Patch

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.