| Message ID | 8bc0fa05-e254-6c85-f48b-e7d353eb9a18@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | rs6000: Fix wrong align passed to build_aligned_type [PR88309] | expand |
On Mon, Apr 8, 2024 at 11:22 AM Kewen.Lin <linkw@linux.ibm.com> wrote: > > Hi, > > As the comments in PR88309 show, there are two oversights > in rs6000_gimple_fold_builtin that pass align in bytes to > build_aligned_type but which actually requires align in > bits, it causes unexpected ICE or hanging in function > is_miss_rate_acceptable due to zero align_unit value. > > This patch is to fix them by converting bytes to bits, add > an assertion on positive align_unit value and notes function > build_aligned_type requires align measured in bits in its > function comment. > > Bootstrapped and regtested on x86_64-redhat-linux, > powerpc64-linux-gnu P8/P9 and powerpc64le-linux-gnu P9 and P10. > > Is it (the generic part code change) ok for trunk? OK > BR, > Kewen > ----- > PR target/88309 > > Co-authored-by: Andrew Pinski <quic_apinski@quicinc.com> > > gcc/ChangeLog: > > * config/rs6000/rs6000-builtin.cc (rs6000_gimple_fold_builtin): Fix > wrong align passed to function build_aligned_type. > * tree-ssa-loop-prefetch.cc (is_miss_rate_acceptable): Add an > assertion to ensure align_unit should be positive. > * tree.cc (build_qualified_type): Update function comments. > > gcc/testsuite/ChangeLog: > > * gcc.target/powerpc/pr88309.c: New test. > --- > gcc/config/rs6000/rs6000-builtin.cc | 4 ++-- > gcc/testsuite/gcc.target/powerpc/pr88309.c | 27 ++++++++++++++++++++++ > gcc/tree-ssa-loop-prefetch.cc | 2 ++ > gcc/tree.cc | 3 ++- > 4 files changed, 33 insertions(+), 3 deletions(-) > create mode 100644 gcc/testsuite/gcc.target/powerpc/pr88309.c > > diff --git a/gcc/config/rs6000/rs6000-builtin.cc b/gcc/config/rs6000/rs6000-builtin.cc > index 6698274031b..e7d6204074c 100644 > --- a/gcc/config/rs6000/rs6000-builtin.cc > +++ b/gcc/config/rs6000/rs6000-builtin.cc > @@ -1900,7 +1900,7 @@ rs6000_gimple_fold_builtin (gimple_stmt_iterator *gsi) > tree lhs_type = TREE_TYPE (lhs); > /* In GIMPLE the type of the MEM_REF specifies the alignment. The > required alignment (power) is 4 bytes regardless of data type. */ > - tree align_ltype = build_aligned_type (lhs_type, 4); > + tree align_ltype = build_aligned_type (lhs_type, 32); > /* POINTER_PLUS_EXPR wants the offset to be of type 'sizetype'. Create > the tree using the value from arg0. The resulting type will match > the type of arg1. */ > @@ -1944,7 +1944,7 @@ rs6000_gimple_fold_builtin (gimple_stmt_iterator *gsi) > tree arg2_type = ptr_type_node; > /* In GIMPLE the type of the MEM_REF specifies the alignment. The > required alignment (power) is 4 bytes regardless of data type. */ > - tree align_stype = build_aligned_type (arg0_type, 4); > + tree align_stype = build_aligned_type (arg0_type, 32); > /* POINTER_PLUS_EXPR wants the offset to be of type 'sizetype'. Create > the tree using the value from arg1. */ > gimple_seq stmts = NULL; > diff --git a/gcc/testsuite/gcc.target/powerpc/pr88309.c b/gcc/testsuite/gcc.target/powerpc/pr88309.c > new file mode 100644 > index 00000000000..c0078cf2b8c > --- /dev/null > +++ b/gcc/testsuite/gcc.target/powerpc/pr88309.c > @@ -0,0 +1,27 @@ > +/* { dg-require-effective-target powerpc_vsx_ok } */ > +/* { dg-options "-mvsx -O2 -fprefetch-loop-arrays" } */ > + > +/* Verify there is no ICE or hanging. */ > + > +#include <altivec.h> > + > +void b(float *c, vector float a, vector float, vector float) > +{ > + vector float d; > + vector char ahbc; > + vec_xst(vec_perm(a, d, ahbc), 0, c); > +} > + > +vector float e(vector unsigned); > + > +void f() { > + float *dst; > + int g = 0; > + for (;; g += 16) { > + vector unsigned m, i; > + vector unsigned n, j; > + vector unsigned k, l; > + b(dst + g * 3, e(m), e(n), e(k)); > + b(dst + (g + 4) * 3, e(i), e(j), e(l)); > + } > +} > diff --git a/gcc/tree-ssa-loop-prefetch.cc b/gcc/tree-ssa-loop-prefetch.cc > index bbd98e03254..70073cc4fe4 100644 > --- a/gcc/tree-ssa-loop-prefetch.cc > +++ b/gcc/tree-ssa-loop-prefetch.cc > @@ -739,6 +739,8 @@ is_miss_rate_acceptable (unsigned HOST_WIDE_INT cache_line_size, > if (delta >= (HOST_WIDE_INT) cache_line_size) > return false; > > + gcc_assert (align_unit > 0); > + > miss_positions = 0; > total_positions = (cache_line_size / align_unit) * distinct_iters; > max_allowed_miss_positions = (ACCEPTABLE_MISS_RATE * total_positions) / 1000; > diff --git a/gcc/tree.cc b/gcc/tree.cc > index f801712c9dd..6f8400e6640 100644 > --- a/gcc/tree.cc > +++ b/gcc/tree.cc > @@ -5689,7 +5689,8 @@ build_qualified_type (tree type, int type_quals MEM_STAT_DECL) > return t; > } > > -/* Create a variant of type T with alignment ALIGN. */ > +/* Create a variant of type T with alignment ALIGN which > + is measured in bits. */ > > tree > build_aligned_type (tree type, unsigned int align) > -- > 2.43.0
on 2024/4/8 18:47, Richard Biener wrote: > On Mon, Apr 8, 2024 at 11:22 AM Kewen.Lin <linkw@linux.ibm.com> wrote: >> >> Hi, >> >> As the comments in PR88309 show, there are two oversights >> in rs6000_gimple_fold_builtin that pass align in bytes to >> build_aligned_type but which actually requires align in >> bits, it causes unexpected ICE or hanging in function >> is_miss_rate_acceptable due to zero align_unit value. >> >> This patch is to fix them by converting bytes to bits, add >> an assertion on positive align_unit value and notes function >> build_aligned_type requires align measured in bits in its >> function comment. >> >> Bootstrapped and regtested on x86_64-redhat-linux, >> powerpc64-linux-gnu P8/P9 and powerpc64le-linux-gnu P9 and P10. >> >> Is it (the generic part code change) ok for trunk? > > OK Thanks, pushed as r14-9850, is it also ok to backport after burn-in time? BR, Kewen > >> BR, >> Kewen >> ----- >> PR target/88309 >> >> Co-authored-by: Andrew Pinski <quic_apinski@quicinc.com> >> >> gcc/ChangeLog: >> >> * config/rs6000/rs6000-builtin.cc (rs6000_gimple_fold_builtin): Fix >> wrong align passed to function build_aligned_type. >> * tree-ssa-loop-prefetch.cc (is_miss_rate_acceptable): Add an >> assertion to ensure align_unit should be positive. >> * tree.cc (build_qualified_type): Update function comments. >> >> gcc/testsuite/ChangeLog: >> >> * gcc.target/powerpc/pr88309.c: New test. >> --- >> gcc/config/rs6000/rs6000-builtin.cc | 4 ++-- >> gcc/testsuite/gcc.target/powerpc/pr88309.c | 27 ++++++++++++++++++++++ >> gcc/tree-ssa-loop-prefetch.cc | 2 ++ >> gcc/tree.cc | 3 ++- >> 4 files changed, 33 insertions(+), 3 deletions(-) >> create mode 100644 gcc/testsuite/gcc.target/powerpc/pr88309.c >> >> diff --git a/gcc/config/rs6000/rs6000-builtin.cc b/gcc/config/rs6000/rs6000-builtin.cc >> index 6698274031b..e7d6204074c 100644 >> --- a/gcc/config/rs6000/rs6000-builtin.cc >> +++ b/gcc/config/rs6000/rs6000-builtin.cc >> @@ -1900,7 +1900,7 @@ rs6000_gimple_fold_builtin (gimple_stmt_iterator *gsi) >> tree lhs_type = TREE_TYPE (lhs); >> /* In GIMPLE the type of the MEM_REF specifies the alignment. The >> required alignment (power) is 4 bytes regardless of data type. */ >> - tree align_ltype = build_aligned_type (lhs_type, 4); >> + tree align_ltype = build_aligned_type (lhs_type, 32); >> /* POINTER_PLUS_EXPR wants the offset to be of type 'sizetype'. Create >> the tree using the value from arg0. The resulting type will match >> the type of arg1. */ >> @@ -1944,7 +1944,7 @@ rs6000_gimple_fold_builtin (gimple_stmt_iterator *gsi) >> tree arg2_type = ptr_type_node; >> /* In GIMPLE the type of the MEM_REF specifies the alignment. The >> required alignment (power) is 4 bytes regardless of data type. */ >> - tree align_stype = build_aligned_type (arg0_type, 4); >> + tree align_stype = build_aligned_type (arg0_type, 32); >> /* POINTER_PLUS_EXPR wants the offset to be of type 'sizetype'. Create >> the tree using the value from arg1. */ >> gimple_seq stmts = NULL; >> diff --git a/gcc/testsuite/gcc.target/powerpc/pr88309.c b/gcc/testsuite/gcc.target/powerpc/pr88309.c >> new file mode 100644 >> index 00000000000..c0078cf2b8c >> --- /dev/null >> +++ b/gcc/testsuite/gcc.target/powerpc/pr88309.c >> @@ -0,0 +1,27 @@ >> +/* { dg-require-effective-target powerpc_vsx_ok } */ >> +/* { dg-options "-mvsx -O2 -fprefetch-loop-arrays" } */ >> + >> +/* Verify there is no ICE or hanging. */ >> + >> +#include <altivec.h> >> + >> +void b(float *c, vector float a, vector float, vector float) >> +{ >> + vector float d; >> + vector char ahbc; >> + vec_xst(vec_perm(a, d, ahbc), 0, c); >> +} >> + >> +vector float e(vector unsigned); >> + >> +void f() { >> + float *dst; >> + int g = 0; >> + for (;; g += 16) { >> + vector unsigned m, i; >> + vector unsigned n, j; >> + vector unsigned k, l; >> + b(dst + g * 3, e(m), e(n), e(k)); >> + b(dst + (g + 4) * 3, e(i), e(j), e(l)); >> + } >> +} >> diff --git a/gcc/tree-ssa-loop-prefetch.cc b/gcc/tree-ssa-loop-prefetch.cc >> index bbd98e03254..70073cc4fe4 100644 >> --- a/gcc/tree-ssa-loop-prefetch.cc >> +++ b/gcc/tree-ssa-loop-prefetch.cc >> @@ -739,6 +739,8 @@ is_miss_rate_acceptable (unsigned HOST_WIDE_INT cache_line_size, >> if (delta >= (HOST_WIDE_INT) cache_line_size) >> return false; >> >> + gcc_assert (align_unit > 0); >> + >> miss_positions = 0; >> total_positions = (cache_line_size / align_unit) * distinct_iters; >> max_allowed_miss_positions = (ACCEPTABLE_MISS_RATE * total_positions) / 1000; >> diff --git a/gcc/tree.cc b/gcc/tree.cc >> index f801712c9dd..6f8400e6640 100644 >> --- a/gcc/tree.cc >> +++ b/gcc/tree.cc >> @@ -5689,7 +5689,8 @@ build_qualified_type (tree type, int type_quals MEM_STAT_DECL) >> return t; >> } >> >> -/* Create a variant of type T with alignment ALIGN. */ >> +/* Create a variant of type T with alignment ALIGN which >> + is measured in bits. */ >> >> tree >> build_aligned_type (tree type, unsigned int align) >> -- >> 2.43.0
On Tue, Apr 9, 2024 at 4:07 AM Kewen.Lin <linkw@linux.ibm.com> wrote: > > on 2024/4/8 18:47, Richard Biener wrote: > > On Mon, Apr 8, 2024 at 11:22 AM Kewen.Lin <linkw@linux.ibm.com> wrote: > >> > >> Hi, > >> > >> As the comments in PR88309 show, there are two oversights > >> in rs6000_gimple_fold_builtin that pass align in bytes to > >> build_aligned_type but which actually requires align in > >> bits, it causes unexpected ICE or hanging in function > >> is_miss_rate_acceptable due to zero align_unit value. > >> > >> This patch is to fix them by converting bytes to bits, add > >> an assertion on positive align_unit value and notes function > >> build_aligned_type requires align measured in bits in its > >> function comment. > >> > >> Bootstrapped and regtested on x86_64-redhat-linux, > >> powerpc64-linux-gnu P8/P9 and powerpc64le-linux-gnu P9 and P10. > >> > >> Is it (the generic part code change) ok for trunk? > > > > OK > > Thanks, pushed as r14-9850, is it also ok to backport after burn-in time? Sure. > BR, > Kewen > > > > >> BR, > >> Kewen > >> ----- > >> PR target/88309 > >> > >> Co-authored-by: Andrew Pinski <quic_apinski@quicinc.com> > >> > >> gcc/ChangeLog: > >> > >> * config/rs6000/rs6000-builtin.cc (rs6000_gimple_fold_builtin): Fix > >> wrong align passed to function build_aligned_type. > >> * tree-ssa-loop-prefetch.cc (is_miss_rate_acceptable): Add an > >> assertion to ensure align_unit should be positive. > >> * tree.cc (build_qualified_type): Update function comments. > >> > >> gcc/testsuite/ChangeLog: > >> > >> * gcc.target/powerpc/pr88309.c: New test. > >> --- > >> gcc/config/rs6000/rs6000-builtin.cc | 4 ++-- > >> gcc/testsuite/gcc.target/powerpc/pr88309.c | 27 ++++++++++++++++++++++ > >> gcc/tree-ssa-loop-prefetch.cc | 2 ++ > >> gcc/tree.cc | 3 ++- > >> 4 files changed, 33 insertions(+), 3 deletions(-) > >> create mode 100644 gcc/testsuite/gcc.target/powerpc/pr88309.c > >> > >> diff --git a/gcc/config/rs6000/rs6000-builtin.cc b/gcc/config/rs6000/rs6000-builtin.cc > >> index 6698274031b..e7d6204074c 100644 > >> --- a/gcc/config/rs6000/rs6000-builtin.cc > >> +++ b/gcc/config/rs6000/rs6000-builtin.cc > >> @@ -1900,7 +1900,7 @@ rs6000_gimple_fold_builtin (gimple_stmt_iterator *gsi) > >> tree lhs_type = TREE_TYPE (lhs); > >> /* In GIMPLE the type of the MEM_REF specifies the alignment. The > >> required alignment (power) is 4 bytes regardless of data type. */ > >> - tree align_ltype = build_aligned_type (lhs_type, 4); > >> + tree align_ltype = build_aligned_type (lhs_type, 32); > >> /* POINTER_PLUS_EXPR wants the offset to be of type 'sizetype'. Create > >> the tree using the value from arg0. The resulting type will match > >> the type of arg1. */ > >> @@ -1944,7 +1944,7 @@ rs6000_gimple_fold_builtin (gimple_stmt_iterator *gsi) > >> tree arg2_type = ptr_type_node; > >> /* In GIMPLE the type of the MEM_REF specifies the alignment. The > >> required alignment (power) is 4 bytes regardless of data type. */ > >> - tree align_stype = build_aligned_type (arg0_type, 4); > >> + tree align_stype = build_aligned_type (arg0_type, 32); > >> /* POINTER_PLUS_EXPR wants the offset to be of type 'sizetype'. Create > >> the tree using the value from arg1. */ > >> gimple_seq stmts = NULL; > >> diff --git a/gcc/testsuite/gcc.target/powerpc/pr88309.c b/gcc/testsuite/gcc.target/powerpc/pr88309.c > >> new file mode 100644 > >> index 00000000000..c0078cf2b8c > >> --- /dev/null > >> +++ b/gcc/testsuite/gcc.target/powerpc/pr88309.c > >> @@ -0,0 +1,27 @@ > >> +/* { dg-require-effective-target powerpc_vsx_ok } */ > >> +/* { dg-options "-mvsx -O2 -fprefetch-loop-arrays" } */ > >> + > >> +/* Verify there is no ICE or hanging. */ > >> + > >> +#include <altivec.h> > >> + > >> +void b(float *c, vector float a, vector float, vector float) > >> +{ > >> + vector float d; > >> + vector char ahbc; > >> + vec_xst(vec_perm(a, d, ahbc), 0, c); > >> +} > >> + > >> +vector float e(vector unsigned); > >> + > >> +void f() { > >> + float *dst; > >> + int g = 0; > >> + for (;; g += 16) { > >> + vector unsigned m, i; > >> + vector unsigned n, j; > >> + vector unsigned k, l; > >> + b(dst + g * 3, e(m), e(n), e(k)); > >> + b(dst + (g + 4) * 3, e(i), e(j), e(l)); > >> + } > >> +} > >> diff --git a/gcc/tree-ssa-loop-prefetch.cc b/gcc/tree-ssa-loop-prefetch.cc > >> index bbd98e03254..70073cc4fe4 100644 > >> --- a/gcc/tree-ssa-loop-prefetch.cc > >> +++ b/gcc/tree-ssa-loop-prefetch.cc > >> @@ -739,6 +739,8 @@ is_miss_rate_acceptable (unsigned HOST_WIDE_INT cache_line_size, > >> if (delta >= (HOST_WIDE_INT) cache_line_size) > >> return false; > >> > >> + gcc_assert (align_unit > 0); > >> + > >> miss_positions = 0; > >> total_positions = (cache_line_size / align_unit) * distinct_iters; > >> max_allowed_miss_positions = (ACCEPTABLE_MISS_RATE * total_positions) / 1000; > >> diff --git a/gcc/tree.cc b/gcc/tree.cc > >> index f801712c9dd..6f8400e6640 100644 > >> --- a/gcc/tree.cc > >> +++ b/gcc/tree.cc > >> @@ -5689,7 +5689,8 @@ build_qualified_type (tree type, int type_quals MEM_STAT_DECL) > >> return t; > >> } > >> > >> -/* Create a variant of type T with alignment ALIGN. */ > >> +/* Create a variant of type T with alignment ALIGN which > >> + is measured in bits. */ > >> > >> tree > >> build_aligned_type (tree type, unsigned int align) > >> -- > >> 2.43.0
diff --git a/gcc/config/rs6000/rs6000-builtin.cc b/gcc/config/rs6000/rs6000-builtin.cc index 6698274031b..e7d6204074c 100644 --- a/gcc/config/rs6000/rs6000-builtin.cc +++ b/gcc/config/rs6000/rs6000-builtin.cc @@ -1900,7 +1900,7 @@ rs6000_gimple_fold_builtin (gimple_stmt_iterator *gsi) tree lhs_type = TREE_TYPE (lhs); /* In GIMPLE the type of the MEM_REF specifies the alignment. The required alignment (power) is 4 bytes regardless of data type. */ - tree align_ltype = build_aligned_type (lhs_type, 4); + tree align_ltype = build_aligned_type (lhs_type, 32); /* POINTER_PLUS_EXPR wants the offset to be of type 'sizetype'. Create the tree using the value from arg0. The resulting type will match the type of arg1. */ @@ -1944,7 +1944,7 @@ rs6000_gimple_fold_builtin (gimple_stmt_iterator *gsi) tree arg2_type = ptr_type_node; /* In GIMPLE the type of the MEM_REF specifies the alignment. The required alignment (power) is 4 bytes regardless of data type. */ - tree align_stype = build_aligned_type (arg0_type, 4); + tree align_stype = build_aligned_type (arg0_type, 32); /* POINTER_PLUS_EXPR wants the offset to be of type 'sizetype'. Create the tree using the value from arg1. */ gimple_seq stmts = NULL; diff --git a/gcc/testsuite/gcc.target/powerpc/pr88309.c b/gcc/testsuite/gcc.target/powerpc/pr88309.c new file mode 100644 index 00000000000..c0078cf2b8c --- /dev/null +++ b/gcc/testsuite/gcc.target/powerpc/pr88309.c @@ -0,0 +1,27 @@ +/* { dg-require-effective-target powerpc_vsx_ok } */ +/* { dg-options "-mvsx -O2 -fprefetch-loop-arrays" } */ + +/* Verify there is no ICE or hanging. */ + +#include <altivec.h> + +void b(float *c, vector float a, vector float, vector float) +{ + vector float d; + vector char ahbc; + vec_xst(vec_perm(a, d, ahbc), 0, c); +} + +vector float e(vector unsigned); + +void f() { + float *dst; + int g = 0; + for (;; g += 16) { + vector unsigned m, i; + vector unsigned n, j; + vector unsigned k, l; + b(dst + g * 3, e(m), e(n), e(k)); + b(dst + (g + 4) * 3, e(i), e(j), e(l)); + } +} diff --git a/gcc/tree-ssa-loop-prefetch.cc b/gcc/tree-ssa-loop-prefetch.cc index bbd98e03254..70073cc4fe4 100644 --- a/gcc/tree-ssa-loop-prefetch.cc +++ b/gcc/tree-ssa-loop-prefetch.cc @@ -739,6 +739,8 @@ is_miss_rate_acceptable (unsigned HOST_WIDE_INT cache_line_size, if (delta >= (HOST_WIDE_INT) cache_line_size) return false; + gcc_assert (align_unit > 0); + miss_positions = 0; total_positions = (cache_line_size / align_unit) * distinct_iters; max_allowed_miss_positions = (ACCEPTABLE_MISS_RATE * total_positions) / 1000; diff --git a/gcc/tree.cc b/gcc/tree.cc index f801712c9dd..6f8400e6640 100644 --- a/gcc/tree.cc +++ b/gcc/tree.cc @@ -5689,7 +5689,8 @@ build_qualified_type (tree type, int type_quals MEM_STAT_DECL) return t; } -/* Create a variant of type T with alignment ALIGN. */ +/* Create a variant of type T with alignment ALIGN which + is measured in bits. */ tree build_aligned_type (tree type, unsigned int align)