PR97849: aarch64: ICE (segfault) during GIMPLE pass: ifcvt

PR97849: aarch64: ICE (segfault) during GIMPLE pass: ifcvt

[Prathamesh Kulkarni]

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

Hi,
For the following test-case (slightly reduced from PR)
int a, b, c;

int g() {
  char i = 0;
  for (c = 0; c <= 8; c++)
    --i;

  while (b) {
    _Bool f = i <= 0;
    a = (a == 0) ? 0 : f / a;
  }
}

The compiler segfaults with -O1 -march=armv8.2-a+sve in ifcvt_local_dce.

IIUC, the issue here is that tree-if-conv.c:predicate_rhs_code
processes the following statement:
iftmp.2_7 = a_lsm.10_11 != 0 ? iftmp.2_13 : 0;
and records <iftmp.2_7, iftmp.2_13> mapping.

However RPO VN eliminates iftmp.2_13:
Removing dead stmt iftmp.2_13 = .COND_DIV (_29, _4, a_lsm.10_11, 0);

and we end up replacing iftmp.2_7 with a dead ssa_name in ifcvt_local_dce:
FOR_EACH_VEC_ELT (redundant_ssa_names, i, name_pair)
    replace_uses_by (name_pair->first, name_pair->second);
  redundant_ssa_names.release ();

resulting in incorrect IR, and segfault down the line.

To avoid clashing of RPO VN with redunant_ssa_names, the patch simply moves
ifcvt_local_dce before do_rpo_vn, which avoids the segfault.
Does that look OK ?
(Altho I guess, doing DCE after VN is better in principle)

Thanks,
Prathamesh

[-- Attachment #2: pr97849-1.diff --]
[-- Type: application/octet-stream, Size: 1169 bytes --]

diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr97849.c b/gcc/testsuite/gcc.dg/tree-ssa/pr97849.c
new file mode 100644
index 00000000000..48ec4db7b30
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tree-ssa/pr97849.c
@@ -0,0 +1,16 @@
+/* { dg-do compile } */
+/* { dg-require-effective-target aarch64_sve } */
+/* { dg-options "-O1 -ftree-vectorize -march=armv8.2-a+sve" } */
+
+int a, b, c;
+
+int g() {
+  char i = 0;
+  for (c = 0; c <= 8; c++)
+    --i;
+
+  while (b) {
+    _Bool f = i <= 0;
+    a = (a == 0) ? 0 : f / a;
+  }
+}
diff --git a/gcc/tree-if-conv.c b/gcc/tree-if-conv.c
index 2062758f40f..efc543829f2 100644
--- a/gcc/tree-if-conv.c
+++ b/gcc/tree-if-conv.c
@@ -3124,10 +3124,11 @@ tree_if_conversion (class loop *loop, vec<gimple *> *preds)
   exit_bbs = BITMAP_ALLOC (NULL);
   bitmap_set_bit (exit_bbs, single_exit (loop)->dest->index);
   bitmap_set_bit (exit_bbs, loop->latch->index);
-  todo |= do_rpo_vn (cfun, loop_preheader_edge (loop), exit_bbs);
 
   /* Delete dead predicate computations.  */
   ifcvt_local_dce (loop);
+
+  todo |= do_rpo_vn (cfun, loop_preheader_edge (loop), exit_bbs);
   BITMAP_FREE (exit_bbs);
 
   todo |= TODO_cleanup_cfg;

Re: PR97849: aarch64: ICE (segfault) during GIMPLE pass: ifcvt

[Richard Biener]

On Wed, 18 Nov 2020, Prathamesh Kulkarni wrote:

> Hi,
> For the following test-case (slightly reduced from PR)
> int a, b, c;
> 
> int g() {
>   char i = 0;
>   for (c = 0; c <= 8; c++)
>     --i;
> 
>   while (b) {
>     _Bool f = i <= 0;
>     a = (a == 0) ? 0 : f / a;
>   }
> }
> 
> The compiler segfaults with -O1 -march=armv8.2-a+sve in ifcvt_local_dce.
> 
> IIUC, the issue here is that tree-if-conv.c:predicate_rhs_code
> processes the following statement:
> iftmp.2_7 = a_lsm.10_11 != 0 ? iftmp.2_13 : 0;
> and records <iftmp.2_7, iftmp.2_13> mapping.
> 
> However RPO VN eliminates iftmp.2_13:
> Removing dead stmt iftmp.2_13 = .COND_DIV (_29, _4, a_lsm.10_11, 0);
> 
> and we end up replacing iftmp.2_7 with a dead ssa_name in ifcvt_local_dce:
> FOR_EACH_VEC_ELT (redundant_ssa_names, i, name_pair)
>     replace_uses_by (name_pair->first, name_pair->second);
>   redundant_ssa_names.release ();
> 
> resulting in incorrect IR, and segfault down the line.
> 
> To avoid clashing of RPO VN with redunant_ssa_names, the patch simply moves
> ifcvt_local_dce before do_rpo_vn, which avoids the segfault.
> Does that look OK ?
> (Altho I guess, doing DCE after VN is better in principle)

Yes, I'd say just moving

  FOR_EACH_VEC_ELT (redundant_ssa_names, i, name_pair)
    replace_uses_by (name_pair->first, name_pair->second);
  redundant_ssa_names.release ();

before rpo_vn makes more sense, no?

OK with that change.
Thanks,
Richard.

> Thanks,
> Prathamesh
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Felix Imend

Re: PR97849: aarch64: ICE (segfault) during GIMPLE pass: ifcvt

[Prathamesh Kulkarni]

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

On Wed, 18 Nov 2020 at 19:13, Richard Biener <rguenther@suse.de> wrote:
>
> On Wed, 18 Nov 2020, Prathamesh Kulkarni wrote:
>
> > Hi,
> > For the following test-case (slightly reduced from PR)
> > int a, b, c;
> >
> > int g() {
> >   char i = 0;
> >   for (c = 0; c <= 8; c++)
> >     --i;
> >
> >   while (b) {
> >     _Bool f = i <= 0;
> >     a = (a == 0) ? 0 : f / a;
> >   }
> > }
> >
> > The compiler segfaults with -O1 -march=armv8.2-a+sve in ifcvt_local_dce.
> >
> > IIUC, the issue here is that tree-if-conv.c:predicate_rhs_code
> > processes the following statement:
> > iftmp.2_7 = a_lsm.10_11 != 0 ? iftmp.2_13 : 0;
> > and records <iftmp.2_7, iftmp.2_13> mapping.
> >
> > However RPO VN eliminates iftmp.2_13:
> > Removing dead stmt iftmp.2_13 = .COND_DIV (_29, _4, a_lsm.10_11, 0);
> >
> > and we end up replacing iftmp.2_7 with a dead ssa_name in ifcvt_local_dce:
> > FOR_EACH_VEC_ELT (redundant_ssa_names, i, name_pair)
> >     replace_uses_by (name_pair->first, name_pair->second);
> >   redundant_ssa_names.release ();
> >
> > resulting in incorrect IR, and segfault down the line.
> >
> > To avoid clashing of RPO VN with redunant_ssa_names, the patch simply moves
> > ifcvt_local_dce before do_rpo_vn, which avoids the segfault.
> > Does that look OK ?
> > (Altho I guess, doing DCE after VN is better in principle)
>
> Yes, I'd say just moving
>
>   FOR_EACH_VEC_ELT (redundant_ssa_names, i, name_pair)
>     replace_uses_by (name_pair->first, name_pair->second);
>   redundant_ssa_names.release ();
>
> before rpo_vn makes more sense, no?
Ah indeed, thanks for the suggestion.
Committed the attached patch after bootstrap+test on x86_64, and
cross testing on aarch64*-*-* and arm*-*-*.

Thanks,
Prathamesh
>
> OK with that change.
> Thanks,
> Richard.
>
> > Thanks,
> > Prathamesh
> >
>
> --
> Richard Biener <rguenther@suse.de>
> SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
> Germany; GF: Felix Imend

[-- Attachment #2: pr97849-3.diff --]
[-- Type: application/octet-stream, Size: 1679 bytes --]

diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr97849.c b/gcc/testsuite/gcc.dg/tree-ssa/pr97849.c
new file mode 100644
index 00000000000..57a31e316a2
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tree-ssa/pr97849.c
@@ -0,0 +1,16 @@
+/* { dg-do compile } */
+/* { dg-options "-O1 -ftree-vectorize" } */
+/* { dg-additional-options "-march=armv8.2-a+sve" { target aarch64*-*-* } } */
+
+int a, b, c;
+
+int g() {
+  char i = 0;
+  for (c = 0; c <= 8; c++)
+    --i;
+
+  while (b) {
+    _Bool f = i <= 0;
+    a = (a == 0) ? 0 : f / a;
+  }
+}
diff --git a/gcc/tree-if-conv.c b/gcc/tree-if-conv.c
index 2062758f40f..93effaa811b 100644
--- a/gcc/tree-if-conv.c
+++ b/gcc/tree-if-conv.c
@@ -2916,12 +2916,6 @@ ifcvt_local_dce (class loop *loop)
   enum gimple_code code;
   use_operand_p use_p;
   imm_use_iterator imm_iter;
-  std::pair <tree, tree> *name_pair;
-  unsigned int i;
-
-  FOR_EACH_VEC_ELT (redundant_ssa_names, i, name_pair)
-    replace_uses_by (name_pair->first, name_pair->second);
-  redundant_ssa_names.release ();
 
   /* The loop has a single BB only.  */
   basic_block bb = loop->header;
@@ -3124,6 +3118,13 @@ tree_if_conversion (class loop *loop, vec<gimple *> *preds)
   exit_bbs = BITMAP_ALLOC (NULL);
   bitmap_set_bit (exit_bbs, single_exit (loop)->dest->index);
   bitmap_set_bit (exit_bbs, loop->latch->index);
+
+  std::pair <tree, tree> *name_pair;
+  unsigned ssa_names_idx;
+  FOR_EACH_VEC_ELT (redundant_ssa_names, ssa_names_idx, name_pair)
+    replace_uses_by (name_pair->first, name_pair->second);
+  redundant_ssa_names.release ();
+
   todo |= do_rpo_vn (cfun, loop_preheader_edge (loop), exit_bbs);
 
   /* Delete dead predicate computations.  */