Re: [PATCH] rs6000: Fix wrong align passed to build_aligned_type [PR88309]

["Kewen.Lin" <linkw@linux.ibm.com>]

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