Message ID | 642de18f-8175-27ca-9a97-d68cd172aa98@linux.ibm.com |
---|---|
State | New |
Headers | show |
Series | rs6000: Fix ICE expanding lxvp and stxvp gimple built-ins [PR101849] | expand |
Hi Peter, LGTM. Still needs maintainer review, of course. :) Bill On 8/10/21 6:37 PM, Peter Bergner wrote: > PR101849 shows we ICE on a test case when we pass a non __vector_pair * > pointer to the __builtin_vsx_lxvp and __builtin_vsx_stxvp built-ins > that is cast to __vector_pair *. The problem is that when we expand > the built-in, the cast has already been removed from gimple and we are > only given the base pointer. The solution used here (which fixes the ICE) > is to catch this case and convert the pointer to a __vector_pair * pointer > when expanding the built-in. > > This passed bootstrap and regression testing on powerpc64le-linux with > no regressions. Ok for mainline? This also affects GCC 11 and 10, so > ok there too after it has baked on trunk for a few days? > > Peter > > > gcc/ > PR target/101849 > * config/rs6000/rs6000-call.c (rs6000_gimple_fold_mma_builtin): Cast > pointer to __vector_pair *. > > gcc/testsuite/ > PR target/101849 > * gcc.target/powerpc/pr101849.c: New test. > > > diff --git a/gcc/config/rs6000/rs6000-call.c b/gcc/config/rs6000/rs6000-call.c > index 904e104c058..d04011c0489 100644 > --- a/gcc/config/rs6000/rs6000-call.c > +++ b/gcc/config/rs6000/rs6000-call.c > @@ -11919,6 +11919,9 @@ rs6000_gimple_fold_mma_builtin (gimple_stmt_iterator *gsi) > tree offset = gimple_call_arg (stmt, 0); > tree ptr = gimple_call_arg (stmt, 1); > tree lhs = gimple_call_lhs (stmt); > + if (TREE_TYPE (TREE_TYPE (ptr)) != vector_pair_type_node) > + ptr = build1 (VIEW_CONVERT_EXPR, > + build_pointer_type (vector_pair_type_node), ptr); > tree mem = build_simple_mem_ref (build2 (POINTER_PLUS_EXPR, > TREE_TYPE (ptr), ptr, offset)); > gimplify_assign (lhs, mem, &new_seq); > @@ -11932,6 +11935,9 @@ rs6000_gimple_fold_mma_builtin (gimple_stmt_iterator *gsi) > tree src = gimple_call_arg (stmt, 0); > tree offset = gimple_call_arg (stmt, 1); > tree ptr = gimple_call_arg (stmt, 2); > + if (TREE_TYPE (TREE_TYPE (ptr)) != vector_pair_type_node) > + ptr = build1 (VIEW_CONVERT_EXPR, > + build_pointer_type (vector_pair_type_node), ptr); > tree mem = build_simple_mem_ref (build2 (POINTER_PLUS_EXPR, > TREE_TYPE (ptr), ptr, offset)); > gimplify_assign (mem, src, &new_seq); > diff --git a/gcc/testsuite/gcc.target/powerpc/pr101849.c b/gcc/testsuite/gcc.target/powerpc/pr101849.c > new file mode 100644 > index 00000000000..6d2e3b79282 > --- /dev/null > +++ b/gcc/testsuite/gcc.target/powerpc/pr101849.c > @@ -0,0 +1,19 @@ > +/* PR target/101849 */ > +/* { dg-do compile } */ > +/* { dg-require-effective-target power10_ok } */ > +/* { dg-options "-O2 -mdejagnu-cpu=power10" } */ > + > +/* Verify we do not ICE on the tests below. */ > + > +__vector_pair vp; > +void > +foo (double *x) > +{ > + vp = __builtin_vsx_lxvp (0, (__vector_pair *)(void *)x); > +} > + > +void > +bar (__vector_pair *src, double *x) > +{ > + __builtin_vsx_stxvp (*src, 0, (__vector_pair *)(void *)x); > +}
On Tue, Aug 10, 2021 at 7:37 PM Peter Bergner <bergner@linux.ibm.com> wrote: > > PR101849 shows we ICE on a test case when we pass a non __vector_pair * > pointer to the __builtin_vsx_lxvp and __builtin_vsx_stxvp built-ins > that is cast to __vector_pair *. The problem is that when we expand > the built-in, the cast has already been removed from gimple and we are > only given the base pointer. The solution used here (which fixes the ICE) > is to catch this case and convert the pointer to a __vector_pair * pointer > when expanding the built-in. > > This passed bootstrap and regression testing on powerpc64le-linux with > no regressions. Ok for mainline? This also affects GCC 11 and 10, so > ok there too after it has baked on trunk for a few days? > > Peter > > > gcc/ > PR target/101849 > * config/rs6000/rs6000-call.c (rs6000_gimple_fold_mma_builtin): Cast > pointer to __vector_pair *. > > gcc/testsuite/ > PR target/101849 > * gcc.target/powerpc/pr101849.c: New test. Okay. Thanks, David
On 8/12/21 1:20 PM, David Edelsohn wrote: > On Tue, Aug 10, 2021 at 7:37 PM Peter Bergner <bergner@linux.ibm.com> wrote: >> gcc/ >> PR target/101849 >> * config/rs6000/rs6000-call.c (rs6000_gimple_fold_mma_builtin): Cast >> pointer to __vector_pair *. >> >> gcc/testsuite/ >> PR target/101849 >> * gcc.target/powerpc/pr101849.c: New test. > > Okay. > > Thanks, David V2: The previous patch was "ok'd" by David, but in the bugzilla, richi made a suggestion that using build2, the ptr's type is ignored and we could just pass in the pointer type we want, which is a little simpler, so that is what I have done with this version, which I think is better than the previous one. I have also expanded the test case a little to test both constant and non-constant offsets. This version passed bootstrap and regression testing on powerpc64le-linux with no regressions. Ok for mainline? This also affects GCC 11 and 10, so ok there too after it has baked on trunk for a few days? ...or do we want to stick with the previous patch? Peter rs6000: Fix ICE expanding lxvp and stxvp gimple built-ins [PR101849] PR101849 shows we ICE on a test case when we pass a non __vector_pair * pointer to the __builtin_vsx_lxvp and __builtin_vsx_stxvp built-ins that is cast to __vector_pair *. The problem is that when we expand the built-in, the cast has already been removed from gimple and we are only given the base pointer. The solution used here (which fixes the ICE) is to always use a __vector_pair * pointer when expanding the built-in. gcc/ PR target/101849 * config/rs6000/rs6000-call.c (rs6000_gimple_fold_mma_builtin): Force pointer to __vector_pair *. gcc/testsuite/ PR target/101849 * gcc.target/powerpc/pr101849.c: New test. diff --git a/gcc/config/rs6000/rs6000-call.c b/gcc/config/rs6000/rs6000-call.c index 904e104c058..90d7171fa46 100644 --- a/gcc/config/rs6000/rs6000-call.c +++ b/gcc/config/rs6000/rs6000-call.c @@ -11919,8 +11919,10 @@ rs6000_gimple_fold_mma_builtin (gimple_stmt_iterator *gsi) tree offset = gimple_call_arg (stmt, 0); tree ptr = gimple_call_arg (stmt, 1); tree lhs = gimple_call_lhs (stmt); - tree mem = build_simple_mem_ref (build2 (POINTER_PLUS_EXPR, - TREE_TYPE (ptr), ptr, offset)); + tree ptr_t = build_pointer_type (vector_pair_type_node); + tree mem = build2 (MEM_REF, vector_pair_type_node, + build2 (POINTER_PLUS_EXPR, ptr_t, ptr, offset), + build_int_cst (ptr_t, 0)); gimplify_assign (lhs, mem, &new_seq); pop_gimplify_context (NULL); gsi_replace_with_seq (gsi, new_seq, true); @@ -11932,8 +11934,10 @@ rs6000_gimple_fold_mma_builtin (gimple_stmt_iterator *gsi) tree src = gimple_call_arg (stmt, 0); tree offset = gimple_call_arg (stmt, 1); tree ptr = gimple_call_arg (stmt, 2); - tree mem = build_simple_mem_ref (build2 (POINTER_PLUS_EXPR, - TREE_TYPE (ptr), ptr, offset)); + tree ptr_t = build_pointer_type (vector_pair_type_node); + tree mem = build2 (MEM_REF, vector_pair_type_node, + build2 (POINTER_PLUS_EXPR, ptr_t, ptr, offset), + build_int_cst (ptr_t, 0)); gimplify_assign (mem, src, &new_seq); pop_gimplify_context (NULL); gsi_replace_with_seq (gsi, new_seq, true); diff --git a/gcc/testsuite/gcc.target/powerpc/pr101849.c b/gcc/testsuite/gcc.target/powerpc/pr101849.c new file mode 100644 index 00000000000..823fbfe9647 --- /dev/null +++ b/gcc/testsuite/gcc.target/powerpc/pr101849.c @@ -0,0 +1,22 @@ +/* PR target/101849 */ +/* { dg-do compile } */ +/* { dg-require-effective-target power10_ok } */ +/* { dg-options "-O2 -mdejagnu-cpu=power10" } */ + +/* Verify we do not ICE on the tests below. */ + +void +foo (__vector_pair *dst, double *x, long offset) +{ + dst[0] = __builtin_vsx_lxvp (0, (__vector_pair *)(void *)x); + dst[1] = __builtin_vsx_lxvp (32, (__vector_pair *)(void *)x); + dst[2] = __builtin_vsx_lxvp (offset, (__vector_pair *)(void *)x); +} + +void +bar (__vector_pair *src, double *x, long offset) +{ + __builtin_vsx_stxvp (src[0], 0, (__vector_pair *)(void *)x); + __builtin_vsx_stxvp (src[1], 32, (__vector_pair *)(void *)x); + __builtin_vsx_stxvp (src[2], offset, (__vector_pair *)(void *)x); +}
Hi Peter, On 8/13/21 12:01 PM, Peter Bergner wrote: > On 8/12/21 1:20 PM, David Edelsohn wrote: >> On Tue, Aug 10, 2021 at 7:37 PM Peter Bergner <bergner@linux.ibm.com> wrote: >>> gcc/ >>> PR target/101849 >>> * config/rs6000/rs6000-call.c (rs6000_gimple_fold_mma_builtin): Cast >>> pointer to __vector_pair *. >>> >>> gcc/testsuite/ >>> PR target/101849 >>> * gcc.target/powerpc/pr101849.c: New test. >> Okay. >> >> Thanks, David > V2: > The previous patch was "ok'd" by David, but in the bugzilla, richi made > a suggestion that using build2, the ptr's type is ignored and we could just > pass in the pointer type we want, which is a little simpler, so that is > what I have done with this version, which I think is better than the > previous one. I have also expanded the test case a little to test both > constant and non-constant offsets. > > This version passed bootstrap and regression testing on powerpc64le-linux > with no regressions. Ok for mainline? This also affects GCC 11 and 10, > so ok there too after it has baked on trunk for a few days? > > ...or do we want to stick with the previous patch? > > Peter Honestly, I don't see how it matters. So far as I can tell, all you've done here is hand-inlined what build_simple_mem_ref would do. So I guess I have a slight preference for your original patch (but with the new test case, of course). Up to you guys. Thanks, Bill > > > > rs6000: Fix ICE expanding lxvp and stxvp gimple built-ins [PR101849] > > PR101849 shows we ICE on a test case when we pass a non __vector_pair * > pointer to the __builtin_vsx_lxvp and __builtin_vsx_stxvp built-ins > that is cast to __vector_pair *. The problem is that when we expand > the built-in, the cast has already been removed from gimple and we are > only given the base pointer. The solution used here (which fixes the ICE) > is to always use a __vector_pair * pointer when expanding the built-in. > > gcc/ > PR target/101849 > * config/rs6000/rs6000-call.c (rs6000_gimple_fold_mma_builtin): Force > pointer to __vector_pair *. > > gcc/testsuite/ > PR target/101849 > * gcc.target/powerpc/pr101849.c: New test. > > > diff --git a/gcc/config/rs6000/rs6000-call.c b/gcc/config/rs6000/rs6000-call.c > index 904e104c058..90d7171fa46 100644 > --- a/gcc/config/rs6000/rs6000-call.c > +++ b/gcc/config/rs6000/rs6000-call.c > @@ -11919,8 +11919,10 @@ rs6000_gimple_fold_mma_builtin (gimple_stmt_iterator *gsi) > tree offset = gimple_call_arg (stmt, 0); > tree ptr = gimple_call_arg (stmt, 1); > tree lhs = gimple_call_lhs (stmt); > - tree mem = build_simple_mem_ref (build2 (POINTER_PLUS_EXPR, > - TREE_TYPE (ptr), ptr, offset)); > + tree ptr_t = build_pointer_type (vector_pair_type_node); > + tree mem = build2 (MEM_REF, vector_pair_type_node, > + build2 (POINTER_PLUS_EXPR, ptr_t, ptr, offset), > + build_int_cst (ptr_t, 0)); > gimplify_assign (lhs, mem, &new_seq); > pop_gimplify_context (NULL); > gsi_replace_with_seq (gsi, new_seq, true); > @@ -11932,8 +11934,10 @@ rs6000_gimple_fold_mma_builtin (gimple_stmt_iterator *gsi) > tree src = gimple_call_arg (stmt, 0); > tree offset = gimple_call_arg (stmt, 1); > tree ptr = gimple_call_arg (stmt, 2); > - tree mem = build_simple_mem_ref (build2 (POINTER_PLUS_EXPR, > - TREE_TYPE (ptr), ptr, offset)); > + tree ptr_t = build_pointer_type (vector_pair_type_node); > + tree mem = build2 (MEM_REF, vector_pair_type_node, > + build2 (POINTER_PLUS_EXPR, ptr_t, ptr, offset), > + build_int_cst (ptr_t, 0)); > gimplify_assign (mem, src, &new_seq); > pop_gimplify_context (NULL); > gsi_replace_with_seq (gsi, new_seq, true); > diff --git a/gcc/testsuite/gcc.target/powerpc/pr101849.c b/gcc/testsuite/gcc.target/powerpc/pr101849.c > new file mode 100644 > index 00000000000..823fbfe9647 > --- /dev/null > +++ b/gcc/testsuite/gcc.target/powerpc/pr101849.c > @@ -0,0 +1,22 @@ > +/* PR target/101849 */ > +/* { dg-do compile } */ > +/* { dg-require-effective-target power10_ok } */ > +/* { dg-options "-O2 -mdejagnu-cpu=power10" } */ > + > +/* Verify we do not ICE on the tests below. */ > + > +void > +foo (__vector_pair *dst, double *x, long offset) > +{ > + dst[0] = __builtin_vsx_lxvp (0, (__vector_pair *)(void *)x); > + dst[1] = __builtin_vsx_lxvp (32, (__vector_pair *)(void *)x); > + dst[2] = __builtin_vsx_lxvp (offset, (__vector_pair *)(void *)x); > +} > + > +void > +bar (__vector_pair *src, double *x, long offset) > +{ > + __builtin_vsx_stxvp (src[0], 0, (__vector_pair *)(void *)x); > + __builtin_vsx_stxvp (src[1], 32, (__vector_pair *)(void *)x); > + __builtin_vsx_stxvp (src[2], offset, (__vector_pair *)(void *)x); > +} > >
On 8/13/21 12:15 PM, Bill Schmidt wrote: > Honestly, I don't see how it matters. So far as I can tell, all you've done > here is hand-inlined what build_simple_mem_ref would do. So I guess I have > a slight preference for your original patch (but with the new test case, > of course). Ok, I ended up pushing the original patch then with the expanded test case. I'll let this bake on trunk for a bit before back porting. Thanks everyone. Peter
diff --git a/gcc/config/rs6000/rs6000-call.c b/gcc/config/rs6000/rs6000-call.c index 904e104c058..d04011c0489 100644 --- a/gcc/config/rs6000/rs6000-call.c +++ b/gcc/config/rs6000/rs6000-call.c @@ -11919,6 +11919,9 @@ rs6000_gimple_fold_mma_builtin (gimple_stmt_iterator *gsi) tree offset = gimple_call_arg (stmt, 0); tree ptr = gimple_call_arg (stmt, 1); tree lhs = gimple_call_lhs (stmt); + if (TREE_TYPE (TREE_TYPE (ptr)) != vector_pair_type_node) + ptr = build1 (VIEW_CONVERT_EXPR, + build_pointer_type (vector_pair_type_node), ptr); tree mem = build_simple_mem_ref (build2 (POINTER_PLUS_EXPR, TREE_TYPE (ptr), ptr, offset)); gimplify_assign (lhs, mem, &new_seq); @@ -11932,6 +11935,9 @@ rs6000_gimple_fold_mma_builtin (gimple_stmt_iterator *gsi) tree src = gimple_call_arg (stmt, 0); tree offset = gimple_call_arg (stmt, 1); tree ptr = gimple_call_arg (stmt, 2); + if (TREE_TYPE (TREE_TYPE (ptr)) != vector_pair_type_node) + ptr = build1 (VIEW_CONVERT_EXPR, + build_pointer_type (vector_pair_type_node), ptr); tree mem = build_simple_mem_ref (build2 (POINTER_PLUS_EXPR, TREE_TYPE (ptr), ptr, offset)); gimplify_assign (mem, src, &new_seq); diff --git a/gcc/testsuite/gcc.target/powerpc/pr101849.c b/gcc/testsuite/gcc.target/powerpc/pr101849.c new file mode 100644 index 00000000000..6d2e3b79282 --- /dev/null +++ b/gcc/testsuite/gcc.target/powerpc/pr101849.c @@ -0,0 +1,19 @@ +/* PR target/101849 */ +/* { dg-do compile } */ +/* { dg-require-effective-target power10_ok } */ +/* { dg-options "-O2 -mdejagnu-cpu=power10" } */ + +/* Verify we do not ICE on the tests below. */ + +__vector_pair vp; +void +foo (double *x) +{ + vp = __builtin_vsx_lxvp (0, (__vector_pair *)(void *)x); +} + +void +bar (__vector_pair *src, double *x) +{ + __builtin_vsx_stxvp (*src, 0, (__vector_pair *)(void *)x); +}