| Message ID | orbm8vzctl.fsf@lxoliva.fsfla.org |
|---|---|
| State | New |
| Headers | show |
| Series | [PR87054] fix unaligned access | expand |
On September 18, 2018 5:47:34 AM GMT+02:00, Alexandre Oliva <oliva@adacore.com> wrote: >Richard, this is the patch you proposed in the PR a while ago, along >with the testcase I'd posted. Please let me know if the commit-message >paragraph below sounds good to you, and then I'll proceed to install >it, >or amend as requested. (do we really want the MEM_REF to carry >stricter >alignment than the base type? I'd have retained the object's alignment >only for looser-than-type alignment) I think so. We want to retain the alignment we'd extracted from the original ref. As you said the indirection is spuirously introduced by the compiler. > >Building an ADDR_EXPR uses the canonical type to build the pointer >type, but then, as we dereference it, we lose track of lax alignment >known to apply to the dereferenced object. This might not be a >problem in general, but it is when the compiler implicitly introduces >address taking and dereferencing, as it does for asm statements, and >as it may do in some loop optimizations. > >Regstrapped on x86_64-linux-gnu. OK. Richard. >From: Richard Biener <rguenther@suse.de> >for gcc/ChangeLog > > PR middle-end/87054 > * gimplify.c (gimplify_expr): Retain alignment of > addressable lvalue in dereference. > >From: Alexandre Oliva <oliva@adacore.com> >for gcc/testsuite/ChangeLog > > PR middle-end/87054 > * gcc.dg/pr87054.c: New. >--- > gcc/gimplify.c | 8 +++++++- > gcc/testsuite/gcc.dg/pr87054.c | 29 +++++++++++++++++++++++++++++ > 2 files changed, 36 insertions(+), 1 deletion(-) > create mode 100644 gcc/testsuite/gcc.dg/pr87054.c > >diff --git a/gcc/gimplify.c b/gcc/gimplify.c >index f0eb04a751ccc..509fc2f3f5be2 100644 >--- a/gcc/gimplify.c >+++ b/gcc/gimplify.c >@@ -12538,9 +12538,15 @@ gimplify_expr (tree *expr_p, gimple_seq >*pre_p, gimple_seq *post_p, > /* An lvalue will do. Take the address of the expression, store it > in a temporary, and replace the expression with an INDIRECT_REF of > that temporary. */ >+ tree ref_alias_type = reference_alias_ptr_type (*expr_p); >+ unsigned int ref_align = get_object_alignment (*expr_p); >+ tree ref_type = TREE_TYPE (*expr_p); > tmp = build_fold_addr_expr_loc (input_location, *expr_p); > gimplify_expr (&tmp, pre_p, post_p, is_gimple_reg, fb_rvalue); >- *expr_p = build_simple_mem_ref (tmp); >+ if (TYPE_ALIGN (ref_type) != ref_align) >+ ref_type = build_aligned_type (ref_type, ref_align); >+ *expr_p = build2 (MEM_REF, ref_type, >+ tmp, build_zero_cst (ref_alias_type)); > } >else if ((fallback & fb_rvalue) && is_gimple_reg_rhs_or_call (*expr_p)) > { >diff --git a/gcc/testsuite/gcc.dg/pr87054.c >b/gcc/testsuite/gcc.dg/pr87054.c >new file mode 100644 >index 0000000000000..4ca2b62d2c7c9 >--- /dev/null >+++ b/gcc/testsuite/gcc.dg/pr87054.c >@@ -0,0 +1,29 @@ >+// { dg-do run } >+// { dg-options "-O2" } >+ >+#ifndef T >+# ifdef __SSE__ >+# define T __int128 >+# else >+# define T long >+# endif >+#endif >+#ifndef R >+# ifdef __SSE__ >+# define R "x" >+# else >+# define R "r" >+# endif >+#endif >+ >+ >+typedef T A; // #define T to long or __int128 >+struct B { char d; A c; } __attribute__((packed)); >+struct B b[50]; // many elements to avoid loop unrolling >+ >+int main () { >+ int i; >+ for (i = 0; i < sizeof(b) / sizeof(*b); i++) { >+ asm ("" : "+" R (b[i].c)); // #define R to "r" on ppc or "x" on >x86_64 >+ } >+}
On Sep 18, 2018, Richard Biener <rguenther@suse.de> wrote: >> +# ifdef __SSE__ >> +# define T __int128 Rainer reported this didn't work on 32-bit x86 with SSE enabled. Here's a patch that fixes it. Ok to install? Tested on x86_64, including such combinations as -m32, -m32/-msse, and -m32/-mno-sse. [PR87054] adjust testcase for 32-bit x86 From: Alexandre Oliva <oliva@adacore.com> The test assumed __int128 to be available whenever __SSE__ was defined, but this assumption doesn't hold on 32-bit x86. Fixed. for gcc/testsuite/ChangeLog PR middle-end/87054 * gcc.dg/pr87054.c: Adjust for no __int128 on x86. --- gcc/testsuite/gcc.dg/pr87054.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gcc/testsuite/gcc.dg/pr87054.c b/gcc/testsuite/gcc.dg/pr87054.c index 4ca2b62d2c7c9..3cb3b7dac6586 100644 --- a/gcc/testsuite/gcc.dg/pr87054.c +++ b/gcc/testsuite/gcc.dg/pr87054.c @@ -2,7 +2,7 @@ // { dg-options "-O2" } #ifndef T -# ifdef __SSE__ +# if __SIZEOF_INT128__ && defined __SSE__ # define T __int128 # else # define T long
On Mon, 24 Sep 2018, Alexandre Oliva wrote: > On Sep 18, 2018, Richard Biener <rguenther@suse.de> wrote: > > >> +# ifdef __SSE__ > >> +# define T __int128 > > Rainer reported this didn't work on 32-bit x86 with SSE enabled. > Here's a patch that fixes it. Ok to install? Tested on x86_64, > including such combinations as -m32, -m32/-msse, and -m32/-mno-sse. Works for me. > > [PR87054] adjust testcase for 32-bit x86 > > From: Alexandre Oliva <oliva@adacore.com> > > The test assumed __int128 to be available whenever __SSE__ was > defined, but this assumption doesn't hold on 32-bit x86. Fixed. > > for gcc/testsuite/ChangeLog > > PR middle-end/87054 > * gcc.dg/pr87054.c: Adjust for no __int128 on x86. > --- > gcc/testsuite/gcc.dg/pr87054.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/gcc/testsuite/gcc.dg/pr87054.c b/gcc/testsuite/gcc.dg/pr87054.c > index 4ca2b62d2c7c9..3cb3b7dac6586 100644 > --- a/gcc/testsuite/gcc.dg/pr87054.c > +++ b/gcc/testsuite/gcc.dg/pr87054.c > @@ -2,7 +2,7 @@ > // { dg-options "-O2" } > > #ifndef T > -# ifdef __SSE__ > +# if __SIZEOF_INT128__ && defined __SSE__ > # define T __int128 > # else > # define T long >
diff --git a/gcc/gimplify.c b/gcc/gimplify.c index f0eb04a751ccc..509fc2f3f5be2 100644 --- a/gcc/gimplify.c +++ b/gcc/gimplify.c @@ -12538,9 +12538,15 @@ gimplify_expr (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p, /* An lvalue will do. Take the address of the expression, store it in a temporary, and replace the expression with an INDIRECT_REF of that temporary. */ + tree ref_alias_type = reference_alias_ptr_type (*expr_p); + unsigned int ref_align = get_object_alignment (*expr_p); + tree ref_type = TREE_TYPE (*expr_p); tmp = build_fold_addr_expr_loc (input_location, *expr_p); gimplify_expr (&tmp, pre_p, post_p, is_gimple_reg, fb_rvalue); - *expr_p = build_simple_mem_ref (tmp); + if (TYPE_ALIGN (ref_type) != ref_align) + ref_type = build_aligned_type (ref_type, ref_align); + *expr_p = build2 (MEM_REF, ref_type, + tmp, build_zero_cst (ref_alias_type)); } else if ((fallback & fb_rvalue) && is_gimple_reg_rhs_or_call (*expr_p)) { diff --git a/gcc/testsuite/gcc.dg/pr87054.c b/gcc/testsuite/gcc.dg/pr87054.c new file mode 100644 index 0000000000000..4ca2b62d2c7c9 --- /dev/null +++ b/gcc/testsuite/gcc.dg/pr87054.c @@ -0,0 +1,29 @@ +// { dg-do run } +// { dg-options "-O2" } + +#ifndef T +# ifdef __SSE__ +# define T __int128 +# else +# define T long +# endif +#endif +#ifndef R +# ifdef __SSE__ +# define R "x" +# else +# define R "r" +# endif +#endif + + +typedef T A; // #define T to long or __int128 +struct B { char d; A c; } __attribute__((packed)); +struct B b[50]; // many elements to avoid loop unrolling + +int main () { + int i; + for (i = 0; i < sizeof(b) / sizeof(*b); i++) { + asm ("" : "+" R (b[i].c)); // #define R to "r" on ppc or "x" on x86_64 + } +}