* Re: [PATCH] Fix PR79256
@ 2017-01-30 10:48 Uros Bizjak
2017-01-30 10:52 ` Richard Biener
0 siblings, 1 reply; 7+ messages in thread
From: Uros Bizjak @ 2017-01-30 10:48 UTC (permalink / raw)
To: gcc-patches; +Cc: Richard Guenther
> 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.
Uros.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Fix PR79256
2017-01-30 10:48 [PATCH] Fix PR79256 Uros Bizjak
@ 2017-01-30 10:52 ` Richard Biener
2017-01-30 10:56 ` Jakub Jelinek
0 siblings, 1 reply; 7+ messages in thread
From: Richard Biener @ 2017-01-30 10:52 UTC (permalink / raw)
To: Uros Bizjak; +Cc: gcc-patches
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Fix PR79256
2017-01-30 10:52 ` Richard Biener
@ 2017-01-30 10:56 ` Jakub Jelinek
2017-01-30 11:22 ` Richard Biener
0 siblings, 1 reply; 7+ messages in thread
From: Jakub Jelinek @ 2017-01-30 10:56 UTC (permalink / raw)
To: Richard Biener; +Cc: Uros Bizjak, gcc-patches
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Fix PR79256
2017-01-30 10:56 ` Jakub Jelinek
@ 2017-01-30 11:22 ` Richard Biener
2017-01-30 11:24 ` Uros Bizjak
0 siblings, 1 reply; 7+ messages in thread
From: Richard Biener @ 2017-01-30 11:22 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: Uros Bizjak, gcc-patches
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Fix PR79256
2017-01-30 11:22 ` Richard Biener
@ 2017-01-30 11:24 ` Uros Bizjak
2017-01-30 11:27 ` Richard Biener
0 siblings, 1 reply; 7+ messages in thread
From: Uros Bizjak @ 2017-01-30 11:24 UTC (permalink / raw)
To: Richard Biener; +Cc: Jakub Jelinek, gcc-patches
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Fix PR79256
2017-01-30 11:24 ` Uros Bizjak
@ 2017-01-30 11:27 ` Richard Biener
0 siblings, 0 replies; 7+ messages in thread
From: Richard Biener @ 2017-01-30 11:27 UTC (permalink / raw)
To: Uros Bizjak; +Cc: Jakub Jelinek, gcc-patches
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] Fix PR79256
@ 2017-01-30 10:35 Richard Biener
0 siblings, 0 replies; 7+ messages in thread
From: Richard Biener @ 2017-01-30 10:35 UTC (permalink / raw)
To: gcc-patches
The following fixes PR79256 in a way suitable for stage4. But the
underlying issue is that our IL advertises bogus alignment for
types like double on i?86 as their alignment inside structures
is 4 bytes, not 8, and double_type_node advertises 64bit alignment.
UBSAN folks invented the min_align_of_type "hack", quite expensive
one, it builds a FIELD_DECL for just frvs target hook (which also
looks really broken), others would be happy with just the type of the
decl.
So after this fix both RTL expansion and get_object_alignment still
compute bogus alignment for *(double *)p accesses. We could fix
up get_object_alignment as well (but given min_align_of_type cost
I'd rather not do that w/o lowering the cost by changning the hook).
And we can fix this properly by fixing the target(s) to not claim
excessive alignment of such modes. Patches attached below
(the x86 fix bootstrapped fine on x86_64, testing is in progress,
I expect code quality fallout and eventually ABI breakage of course).
Bootstrapped on x86_64-unknown-linux-gnu, testing in progress.
Note RTL expansion is broken since forever checked 3.4.[06] (x86_64
is new in that release).
I do think the proper fix is in the targets. double_type_node
may not advertise 64bit alignment.
Richard.
2017-01-30 Richard Biener <rguenther@suse.de>
PR tree-optimization/79256
* targhooks.c (default_builtin_vector_alignment_reachable): Honor
BIGGEST_FIELD_ALIGNMENT and ADJUST_FIELD_ALIGN to fix up bogus
alignment on TYPE.
* tree.c (build_aligned_type): Set TYPE_USER_ALIGN.
Index: gcc/targhooks.c
===================================================================
--- gcc/targhooks.c (revision 244974)
+++ gcc/targhooks.c (working copy)
@@ -1130,9 +1130,14 @@ default_vector_alignment (const_tree typ
/* By default assume vectors of element TYPE require a multiple of the natural
alignment of TYPE. TYPE is naturally aligned if IS_PACKED is false. */
bool
-default_builtin_vector_alignment_reachable (const_tree /*type*/, bool is_packed)
+default_builtin_vector_alignment_reachable (const_tree type, bool is_packed)
{
- return ! is_packed;
+ if (is_packed)
+ return false;
+
+ /* If TYPE can be differently aligned in field context we have to punt
+ as TYPE may have wrong TYPE_ALIGN here (PR79278). */
+ return min_align_of_type (CONST_CAST_TREE (type)) == TYPE_ALIGN_UNIT (type);
}
/* By default, assume that a target supports any factor of misalignment
Index: gcc/tree.c
===================================================================
--- gcc/tree.c (revision 244974)
+++ gcc/tree.c (working copy)
@@ -6684,6 +6684,7 @@ build_aligned_type (tree type, unsigned
t = build_variant_type_copy (type);
SET_TYPE_ALIGN (t, align);
+ TYPE_USER_ALIGN (t) = 1;
return t;
}
2017-01-30 Richard Biener <rguenther@suse.de>
PR tree-optimization/79256
PR middle-end/79278
* builtins.c (get_object_alignment_2): Use min_align_of_type
to extract alignment for MEM_REFs to honor BIGGEST_FIELD_ALIGNMENT
and ADJUST_FIELD_ALIGN.
Index: gcc/builtins.c
===================================================================
--- gcc/builtins.c (revision 244974)
+++ gcc/builtins.c (working copy)
@@ -334,9 +334,11 @@ get_object_alignment_2 (tree exp, unsign
Do so only if get_pointer_alignment_1 did not reveal absolute
alignment knowledge and if using that alignment would
improve the situation. */
+ unsigned int talign;
if (!addr_p && !known_alignment
- && TYPE_ALIGN (TREE_TYPE (exp)) > align)
- align = TYPE_ALIGN (TREE_TYPE (exp));
+ && (talign = min_align_of_type (TREE_TYPE (exp)) * BITS_PER_UNIT)
+ && talign > align)
+ align = talign;
else
{
/* Else adjust bitpos accordingly. */
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);
/* Add any extra modes needed to represent the condition code.
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2017-01-30 11:27 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-01-30 10:48 [PATCH] Fix PR79256 Uros Bizjak
2017-01-30 10:52 ` Richard Biener
2017-01-30 10:56 ` Jakub Jelinek
2017-01-30 11:22 ` Richard Biener
2017-01-30 11:24 ` Uros Bizjak
2017-01-30 11:27 ` Richard Biener
-- strict thread matches above, loose matches on Subject: below --
2017-01-30 10:35 Richard Biener
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).