[PATCH] PR tree-optimization/97546 Bail out of find_bswap_or_nop on non-INTEGER_CST sizes

[PATCH] PR tree-optimization/97546 Bail out of find_bswap_or_nop on non-INTEGER_CST sizes

[Kyrylo Tkachov]

[-- Attachment #1: Type: text/plain, Size: 583 bytes --]

Hi all,

This patch fixes the ICE in the PR by bailing out of find_bswap_or_nop on poly_int sizes.
I don't think it intends to handle them and from my reading of the code it's the most appropriate place to reject them
here rather than in the callers.

Bootstrapped and tested on aarch64-none-linux-gnu.

Ok for trunk?
Thanks,
Kyrill

gcc/
	PR tree-optimization/97546
	* gimple-ssa-store-merging.c (find_bswap_or_nop): Return NULL if type is
	not INTEGER_CST.

gcc/testsuite/
	PR tree-optimization/97546
	* gcc.target/aarch64/sve/acle/general/pr97546.c: New test.

[-- Attachment #2: sm-poly.patch --]
[-- Type: application/octet-stream, Size: 2113 bytes --]

diff --git a/gcc/gimple-ssa-store-merging.c b/gcc/gimple-ssa-store-merging.c
index 28fc2e282befbb131029b2e47ee9aaeb530d787d..195dfdd50c73c1a9642accb9897a878aa0e03159 100644
--- a/gcc/gimple-ssa-store-merging.c
+++ b/gcc/gimple-ssa-store-merging.c
@@ -851,12 +851,16 @@ find_bswap_or_nop_finalize (struct symbolic_number *n, uint64_t *cmpxchg,
 gimple *
 find_bswap_or_nop (gimple *stmt, struct symbolic_number *n, bool *bswap)
 {
+  tree type_size = TYPE_SIZE_UNIT (gimple_expr_type (stmt));
+  if (!cst_and_fits_in_hwi (type_size))
+    return NULL;
+
   /* The last parameter determines the depth search limit.  It usually
      correlates directly to the number n of bytes to be touched.  We
      increase that number by 2 * (log2(n) + 1) here in order to also
      cover signed -> unsigned conversions of the src operand as can be seen
      in libgcc, and for initial shift/and operation of the src operand.  */
-  int limit = TREE_INT_CST_LOW (TYPE_SIZE_UNIT (gimple_expr_type (stmt)));
+  int limit = TREE_INT_CST_LOW (type_size);
   limit += 2 * (1 + (int) ceil_log2 ((unsigned HOST_WIDE_INT) limit));
   gimple *ins_stmt = find_bswap_or_nop_1 (stmt, n, limit);
 
diff --git a/gcc/testsuite/gcc.target/aarch64/sve/acle/general/pr97546.c b/gcc/testsuite/gcc.target/aarch64/sve/acle/general/pr97546.c
new file mode 100644
index 0000000000000000000000000000000000000000..25707cd280400e00fe152beb7ccaea144418709e
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/sve/acle/general/pr97546.c
@@ -0,0 +1,22 @@
+/* { dg-options "-O2" } */
+
+#include <arm_sve.h>
+
+static svbool_t visinf_vo_vf(svfloat32_t d)
+{
+  return svcmpeq_n_f32 (svptrue_b8 (),
+                        svabs_f32_x (svptrue_b8 (), d),
+                        __builtin_inff ());
+}
+
+const svint32_t _ZGVsNxv_ilogbf(svfloat32_t d)
+{
+  svint32_t e = svreinterpret_s32_f32 (svdup_n_f32 (0.0f));
+  e = svsel_s32 (svcmpne_f32 (svptrue_b8(), d, d),
+                 svdup_n_s32 (2147483647),
+                 e);
+  e = svsel_s32 (visinf_vo_vf (d),
+                 svdup_n_s32 (0x7fffffff),
+                 e);
+  return e;
+}

Re: [PATCH] PR tree-optimization/97546 Bail out of find_bswap_or_nop on non-INTEGER_CST sizes

[Jakub Jelinek]

On Mon, Oct 26, 2020 at 09:20:42AM +0000, Kyrylo Tkachov via Gcc-patches wrote:
> This patch fixes the ICE in the PR by bailing out of find_bswap_or_nop on poly_int sizes.
> I don't think it intends to handle them and from my reading of the code it's the most appropriate place to reject them
> here rather than in the callers.
> 
> Bootstrapped and tested on aarch64-none-linux-gnu.
> 
> Ok for trunk?
> Thanks,
> Kyrill
> 
> gcc/
> 	PR tree-optimization/97546
> 	* gimple-ssa-store-merging.c (find_bswap_or_nop): Return NULL if type is
> 	not INTEGER_CST.

I think better use tree_fits_uhwi_p instead of cst_and_fits_hwi and
instead of TREE_INT_CST_LOW use tree_to_uhwi.
TYPE_SIZE_UNIT which doesn't fit into uhwi but fits into shwi is something
that really shouldn't appear.
Otherwise LGTM.

> gcc/testsuite/
> 	PR tree-optimization/97546
> 	* gcc.target/aarch64/sve/acle/general/pr97546.c: New test.



	Jakub

RE: [PATCH] PR tree-optimization/97546 Bail out of find_bswap_or_nop on non-INTEGER_CST sizes

[Kyrylo Tkachov]

[-- Attachment #1: Type: text/plain, Size: 1376 bytes --]



> -----Original Message-----
> From: Jakub Jelinek <jakub@redhat.com>
> Sent: 26 October 2020 09:32
> To: Kyrylo Tkachov <Kyrylo.Tkachov@arm.com>
> Cc: gcc-patches@gcc.gnu.org
> Subject: Re: [PATCH] PR tree-optimization/97546 Bail out of
> find_bswap_or_nop on non-INTEGER_CST sizes
> 
> On Mon, Oct 26, 2020 at 09:20:42AM +0000, Kyrylo Tkachov via Gcc-patches
> wrote:
> > This patch fixes the ICE in the PR by bailing out of find_bswap_or_nop on
> poly_int sizes.
> > I don't think it intends to handle them and from my reading of the code it's
> the most appropriate place to reject them
> > here rather than in the callers.
> >
> > Bootstrapped and tested on aarch64-none-linux-gnu.
> >
> > Ok for trunk?
> > Thanks,
> > Kyrill
> >
> > gcc/
> > 	PR tree-optimization/97546
> > 	* gimple-ssa-store-merging.c (find_bswap_or_nop): Return NULL if
> type is
> > 	not INTEGER_CST.
> 
> I think better use tree_fits_uhwi_p instead of cst_and_fits_hwi and
> instead of TREE_INT_CST_LOW use tree_to_uhwi.
> TYPE_SIZE_UNIT which doesn't fit into uhwi but fits into shwi is something
> that really shouldn't appear.
> Otherwise LGTM.

Thanks, that makes sense.
Is the attached patch ok?
Kyrill

> 
> > gcc/testsuite/
> > 	PR tree-optimization/97546
> > 	* gcc.target/aarch64/sve/acle/general/pr97546.c: New test.
> 
> 
> 
> 	Jakub


[-- Attachment #2: sm-poly.patch --]
[-- Type: application/octet-stream, Size: 1975 bytes --]

diff --git a/gcc/gimple-ssa-store-merging.c b/gcc/gimple-ssa-store-merging.c
index 28fc2e2..d2a069f 100644
--- a/gcc/gimple-ssa-store-merging.c
+++ b/gcc/gimple-ssa-store-merging.c
@@ -851,12 +851,16 @@ find_bswap_or_nop_finalize (struct symbolic_number *n, uint64_t *cmpxchg,
 gimple *
 find_bswap_or_nop (gimple *stmt, struct symbolic_number *n, bool *bswap)
 {
+  tree type_size = TYPE_SIZE_UNIT (gimple_expr_type (stmt));
+  if (!tree_fits_uhwi_p  (type_size))
+    return NULL;
+
   /* The last parameter determines the depth search limit.  It usually
      correlates directly to the number n of bytes to be touched.  We
      increase that number by 2 * (log2(n) + 1) here in order to also
      cover signed -> unsigned conversions of the src operand as can be seen
      in libgcc, and for initial shift/and operation of the src operand.  */
-  int limit = TREE_INT_CST_LOW (TYPE_SIZE_UNIT (gimple_expr_type (stmt)));
+  int limit = tree_to_uhwi (type_size);
   limit += 2 * (1 + (int) ceil_log2 ((unsigned HOST_WIDE_INT) limit));
   gimple *ins_stmt = find_bswap_or_nop_1 (stmt, n, limit);
 
diff --git a/gcc/testsuite/gcc.target/aarch64/sve/acle/general/pr97546.c b/gcc/testsuite/gcc.target/aarch64/sve/acle/general/pr97546.c
new file mode 100644
index 0000000..25707cd
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/sve/acle/general/pr97546.c
@@ -0,0 +1,22 @@
+/* { dg-options "-O2" } */
+
+#include <arm_sve.h>
+
+static svbool_t visinf_vo_vf(svfloat32_t d)
+{
+  return svcmpeq_n_f32 (svptrue_b8 (),
+                        svabs_f32_x (svptrue_b8 (), d),
+                        __builtin_inff ());
+}
+
+const svint32_t _ZGVsNxv_ilogbf(svfloat32_t d)
+{
+  svint32_t e = svreinterpret_s32_f32 (svdup_n_f32 (0.0f));
+  e = svsel_s32 (svcmpne_f32 (svptrue_b8(), d, d),
+                 svdup_n_s32 (2147483647),
+                 e);
+  e = svsel_s32 (visinf_vo_vf (d),
+                 svdup_n_s32 (0x7fffffff),
+                 e);
+  return e;
+}

Re: [PATCH] PR tree-optimization/97546 Bail out of find_bswap_or_nop on non-INTEGER_CST sizes

[Jakub Jelinek]

On Mon, Oct 26, 2020 at 11:32:43AM +0000, Kyrylo Tkachov wrote:
> Thanks, that makes sense.
> Is the attached patch ok?

--- a/gcc/gimple-ssa-store-merging.c
+++ b/gcc/gimple-ssa-store-merging.c
@@ -851,12 +851,16 @@ find_bswap_or_nop_finalize (struct symbolic_number *n, uint64_t *cmpxchg,
 gimple *
 find_bswap_or_nop (gimple *stmt, struct symbolic_number *n, bool *bswap)
 {
+  tree type_size = TYPE_SIZE_UNIT (gimple_expr_type (stmt));
+  if (!tree_fits_uhwi_p  (type_size))
+    return NULL;

Just one space before ( above.  Ok for trunk with that nit fixed.
Thanks.

	Jakub