| Message ID | ri6ed6kntue.fsf@virgil.suse.cz |
|---|---|
| State | New |
| Headers | show |
| Series | sra: Avoid risking x87 magling binary representation of a replacement (PR 58416) | expand |
On Mon, 19 Aug 2024, Martin Jambor wrote: > Hi, > > PR 58416 shows that storing non-floating point data to floating point > scalar registers can lead to miscompilations when the data is > normalized or otherwise processed upon loading to a register. To > avoid that risk, this patch detects situations where we have multiple > types and a we decide to represent the data in a type with a mode that > is known to not be able to transfer actual bits reliably using the new > TARGET_MODE_CAN_TRANSFER_BITS hook. > > Bootstrapped and tested on x86_64-linux. OK for trunk? OK (well, you know SRA best). > Any back-ports to release branches would of course need a back-port of > the hook itself, unfortunately. Of course. Thanks, Richard. > Thanks, > > Martin > > > gcc/ChangeLog: > > 2024-08-19 Martin Jambor <mjambor@suse.cz> > > PR target/58416 > * tree-sra.cc (types_risk_mangled_binary_repr_p): New function. > (sort_and_splice_var_accesses): Use it. > (propagate_subaccesses_from_rhs): Likewise. > > gcc/testsuite/ChangeLog: > > 2024-08-19 Martin Jambor <mjambor@suse.cz> > > PR target/58416 > * gcc.dg/torture/pr58416.c: New test. > --- > gcc/testsuite/gcc.dg/torture/pr58416.c | 32 ++++++++++++++++++++++++++ > gcc/tree-sra.cc | 28 +++++++++++++++++++++- > 2 files changed, 59 insertions(+), 1 deletion(-) > create mode 100644 gcc/testsuite/gcc.dg/torture/pr58416.c > > diff --git a/gcc/testsuite/gcc.dg/torture/pr58416.c b/gcc/testsuite/gcc.dg/torture/pr58416.c > new file mode 100644 > index 00000000000..0922b0e7089 > --- /dev/null > +++ b/gcc/testsuite/gcc.dg/torture/pr58416.c > @@ -0,0 +1,32 @@ > +/* { dg-do run } */ > + > +struct s { > + char s[sizeof(long double)]; > +}; > + > +union u { > + long double d; > + struct s s; > +}; > + > +int main() > +{ > + union u x = {0}; > +#if __SIZEOF_LONG_DOUBLE__ == 16 > + x.s = (struct s){"xxxxxxxxxxxxxxxx"}; > +#elif __SIZEOF_LONG_DOUBLE__ == 12 > + x.s = (struct s){"xxxxxxxxxxxx"}; > +#elif __SIZEOF_LONG_DOUBLE__ == 8 > + x.s = (struct s){"xxxxxxxx"}; > +#elif __SIZEOF_LONG_DOUBLE__ == 4 > + x.s = (struct s){"xxxx"}; > +#endif > + > + union u y = x; > + > + for (unsigned char *p = (unsigned char *)&y + sizeof y; > + p-- > (unsigned char *)&y;) > + if (*p != (unsigned char)'x') > + __builtin_abort (); > + return 0; > +} > diff --git a/gcc/tree-sra.cc b/gcc/tree-sra.cc > index 8040b0c5645..64e2f007d68 100644 > --- a/gcc/tree-sra.cc > +++ b/gcc/tree-sra.cc > @@ -2335,6 +2335,19 @@ same_access_path_p (tree exp1, tree exp2) > return true; > } > > +/* Return true when either T1 is a type that, when loaded into a register and > + stored back to memory will yield the same bits or when both T1 and T2 are > + compatible. */ > + > +static bool > +types_risk_mangled_binary_repr_p (tree t1, tree t2) > +{ > + if (mode_can_transfer_bits (TYPE_MODE (t1))) > + return false; > + > + return !types_compatible_p (t1, t2); > +} > + > /* Sort all accesses for the given variable, check for partial overlaps and > return NULL if there are any. If there are none, pick a representative for > each combination of offset and size and create a linked list out of them. > @@ -2461,6 +2474,17 @@ sort_and_splice_var_accesses (tree var) > } > unscalarizable_region = true; > } > + else if (types_risk_mangled_binary_repr_p (access->type, ac2->type)) > + { > + if (dump_file && (dump_flags & TDF_DETAILS)) > + { > + fprintf (dump_file, "Cannot scalarize the following access " > + "because data would be held in a mode which is not " > + "guaranteed to preserve all bits.\n "); > + dump_access (dump_file, access, false); > + } > + unscalarizable_region = true; > + } > > if (grp_same_access_path > && !same_access_path_p (access->expr, ac2->expr)) > @@ -3127,7 +3151,9 @@ propagate_subaccesses_from_rhs (struct access *lacc, struct access *racc) > ret = true; > subtree_mark_written_and_rhs_enqueue (lacc); > } > - if (!lacc->first_child && !racc->first_child) > + if (!lacc->first_child > + && !racc->first_child > + && !types_risk_mangled_binary_repr_p (racc->type, lacc->type)) > { > /* We are about to change the access type from aggregate to scalar, > so we need to put the reverse flag onto the access, if any. */ >
diff --git a/gcc/testsuite/gcc.dg/torture/pr58416.c b/gcc/testsuite/gcc.dg/torture/pr58416.c new file mode 100644 index 00000000000..0922b0e7089 --- /dev/null +++ b/gcc/testsuite/gcc.dg/torture/pr58416.c @@ -0,0 +1,32 @@ +/* { dg-do run } */ + +struct s { + char s[sizeof(long double)]; +}; + +union u { + long double d; + struct s s; +}; + +int main() +{ + union u x = {0}; +#if __SIZEOF_LONG_DOUBLE__ == 16 + x.s = (struct s){"xxxxxxxxxxxxxxxx"}; +#elif __SIZEOF_LONG_DOUBLE__ == 12 + x.s = (struct s){"xxxxxxxxxxxx"}; +#elif __SIZEOF_LONG_DOUBLE__ == 8 + x.s = (struct s){"xxxxxxxx"}; +#elif __SIZEOF_LONG_DOUBLE__ == 4 + x.s = (struct s){"xxxx"}; +#endif + + union u y = x; + + for (unsigned char *p = (unsigned char *)&y + sizeof y; + p-- > (unsigned char *)&y;) + if (*p != (unsigned char)'x') + __builtin_abort (); + return 0; +} diff --git a/gcc/tree-sra.cc b/gcc/tree-sra.cc index 8040b0c5645..64e2f007d68 100644 --- a/gcc/tree-sra.cc +++ b/gcc/tree-sra.cc @@ -2335,6 +2335,19 @@ same_access_path_p (tree exp1, tree exp2) return true; } +/* Return true when either T1 is a type that, when loaded into a register and + stored back to memory will yield the same bits or when both T1 and T2 are + compatible. */ + +static bool +types_risk_mangled_binary_repr_p (tree t1, tree t2) +{ + if (mode_can_transfer_bits (TYPE_MODE (t1))) + return false; + + return !types_compatible_p (t1, t2); +} + /* Sort all accesses for the given variable, check for partial overlaps and return NULL if there are any. If there are none, pick a representative for each combination of offset and size and create a linked list out of them. @@ -2461,6 +2474,17 @@ sort_and_splice_var_accesses (tree var) } unscalarizable_region = true; } + else if (types_risk_mangled_binary_repr_p (access->type, ac2->type)) + { + if (dump_file && (dump_flags & TDF_DETAILS)) + { + fprintf (dump_file, "Cannot scalarize the following access " + "because data would be held in a mode which is not " + "guaranteed to preserve all bits.\n "); + dump_access (dump_file, access, false); + } + unscalarizable_region = true; + } if (grp_same_access_path && !same_access_path_p (access->expr, ac2->expr)) @@ -3127,7 +3151,9 @@ propagate_subaccesses_from_rhs (struct access *lacc, struct access *racc) ret = true; subtree_mark_written_and_rhs_enqueue (lacc); } - if (!lacc->first_child && !racc->first_child) + if (!lacc->first_child + && !racc->first_child + && !types_risk_mangled_binary_repr_p (racc->type, lacc->type)) { /* We are about to change the access type from aggregate to scalar, so we need to put the reverse flag onto the access, if any. */