[PATCH] [PR 81245] Fix tree-if-conv calling of update_stmt after fold_stmt

[PATCH] [PR 81245] Fix tree-if-conv calling of update_stmt after fold_stmt

[Andrew Pinski]

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

Hi,
  As described in the bug, tree-if-conv is calling update_stmt on an
old stmt which might have been removed from the IR already
(transforming of an assignment to a call in this case).  This fixes
the problem by calling update_stmt on the new statement that fold_stmt
might have created.

OK?  Bootstrapped and tested on aarch64-linux-gnu with no regressions.

Thanks,
Andrew Pinski
ChangeLog:
* tree-if-conv.c (predicate_scalar_phi): Update new_stmt if fold_stmt
returned true.

testsuite/ChangeLog:
* gcc.dg/torture/pr81245.c: New testcase.

[-- Attachment #2: fixifcvt.diff.txt --]
[-- Type: text/plain, Size: 1214 bytes --]

Index: testsuite/gcc.dg/torture/pr81245.c
===================================================================
--- testsuite/gcc.dg/torture/pr81245.c	(nonexistent)
+++ testsuite/gcc.dg/torture/pr81245.c	(working copy)
@@ -0,0 +1,16 @@
+/* { dg-options "-ffast-math" } */
+/* { dg-do compile } */
+/* This test used to crash the vectorizer as the ifconvert pass
+   used to convert the if to copysign but called update_stmt on
+   the old statement after calling fold_stmt. */
+double sg[18];
+void f(void)
+{
+  for (int i = 0 ;i < 18;i++)
+  {
+    if (sg[i] < 0.0)
+      sg[i] = -1.0;
+    else
+      sg[i] = 1.0;
+  }
+}
Index: tree-if-conv.c
===================================================================
--- tree-if-conv.c	(revision 249769)
+++ tree-if-conv.c	(working copy)
@@ -1853,7 +1853,8 @@
       new_stmt = gimple_build_assign (res, rhs);
       gsi_insert_before (gsi, new_stmt, GSI_SAME_STMT);
       gimple_stmt_iterator new_gsi = gsi_for_stmt (new_stmt);
-      fold_stmt (&new_gsi, ifcvt_follow_ssa_use_edges);
+      if (fold_stmt (&new_gsi, ifcvt_follow_ssa_use_edges))
+	new_stmt = gsi_stmt (new_gsi);
       update_stmt (new_stmt);
 
       if (dump_file && (dump_flags & TDF_DETAILS))

Re: [PATCH] [PR 81245] Fix tree-if-conv calling of update_stmt after fold_stmt

[Richard Biener]

On Thu, Jun 29, 2017 at 10:12 PM, Andrew Pinski <pinskia@gmail.com> wrote:
> Hi,
>   As described in the bug, tree-if-conv is calling update_stmt on an
> old stmt which might have been removed from the IR already
> (transforming of an assignment to a call in this case).  This fixes
> the problem by calling update_stmt on the new statement that fold_stmt
> might have created.
>
> OK?  Bootstrapped and tested on aarch64-linux-gnu with no regressions.

update_stmt is not necessary when fold_stmt doesn't return true as
gsi_insert_before already updates the stmt.

Thus ok with moving update_stmt under the conditional.

Thanks,
Richard.

> Thanks,
> Andrew Pinski
> ChangeLog:
> * tree-if-conv.c (predicate_scalar_phi): Update new_stmt if fold_stmt
> returned true.
>
> testsuite/ChangeLog:
> * gcc.dg/torture/pr81245.c: New testcase.

Re: [PATCH] [PR 81245] Fix tree-if-conv calling of update_stmt after fold_stmt

[Andrew Pinski]

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

On Fri, Jun 30, 2017 at 1:20 AM, Richard Biener
<richard.guenther@gmail.com> wrote:
> On Thu, Jun 29, 2017 at 10:12 PM, Andrew Pinski <pinskia@gmail.com> wrote:
>> Hi,
>>   As described in the bug, tree-if-conv is calling update_stmt on an
>> old stmt which might have been removed from the IR already
>> (transforming of an assignment to a call in this case).  This fixes
>> the problem by calling update_stmt on the new statement that fold_stmt
>> might have created.
>>
>> OK?  Bootstrapped and tested on aarch64-linux-gnu with no regressions.
>
> update_stmt is not necessary when fold_stmt doesn't return true as
> gsi_insert_before already updates the stmt.
>
> Thus ok with moving update_stmt under the conditional.

This is what I applied.

Thanks,
Andrew

>
> Thanks,
> Richard.
>
>> Thanks,
>> Andrew Pinski
>> ChangeLog:
>> * tree-if-conv.c (predicate_scalar_phi): Update new_stmt if fold_stmt
>> returned true.
>>
>> testsuite/ChangeLog:
>> * gcc.dg/torture/pr81245.c: New testcase.

[-- Attachment #2: fixifcvt.diff.txt --]
[-- Type: text/plain, Size: 1257 bytes --]

Index: testsuite/gcc.dg/torture/pr81245.c
===================================================================
--- testsuite/gcc.dg/torture/pr81245.c	(nonexistent)
+++ testsuite/gcc.dg/torture/pr81245.c	(working copy)
@@ -0,0 +1,16 @@
+/* { dg-options "-ffast-math" } */
+/* { dg-do compile } */
+/* This test used to crash the vectorizer as the ifconvert pass
+   used to convert the if to copysign but called update_stmt on
+   the old statement after calling fold_stmt. */
+double sg[18];
+void f(void)
+{
+  for (int i = 0 ;i < 18;i++)
+  {
+    if (sg[i] < 0.0)
+      sg[i] = -1.0;
+    else
+      sg[i] = 1.0;
+  }
+}
Index: tree-if-conv.c
===================================================================
--- tree-if-conv.c	(revision 249844)
+++ tree-if-conv.c	(working copy)
@@ -1853,8 +1853,11 @@
       new_stmt = gimple_build_assign (res, rhs);
       gsi_insert_before (gsi, new_stmt, GSI_SAME_STMT);
       gimple_stmt_iterator new_gsi = gsi_for_stmt (new_stmt);
-      fold_stmt (&new_gsi, ifcvt_follow_ssa_use_edges);
-      update_stmt (new_stmt);
+      if (fold_stmt (&new_gsi, ifcvt_follow_ssa_use_edges))
+	{
+	  new_stmt = gsi_stmt (new_gsi);
+	  update_stmt (new_stmt);
+	}
 
       if (dump_file && (dump_flags & TDF_DETAILS))
 	{