[i386] [PATCH] Fix ICE when lhs is NULL [PR target/100660]

[i386] [PATCH] Fix ICE when lhs is NULL [PR target/100660]

[Hongtao Liu]

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

Hi:
  In folding target-specific builtin, when lhs is NULL, create a
temporary variable for it.
  Bootstrapped and regtested on x86_64-linux-gnu{-m32,}

gcc/ChangeLog:
        PR target/100660
        * config/i386/i386.c (ix86_gimple_fold_builtin): Create a tmp
        variable for lhs when it doesn't exist.

gcc/testsuite/ChangeLog:
        PR target/100660
        * gcc.target/i386/pr100660.c: New test.



-- 
BR,
Hongtao

[-- Attachment #2: 0001-Fix-ICE-when-lhs-is-NULL.patch --]
[-- Type: text/x-patch, Size: 1763 bytes --]

From b32791645429e3e25c46f56e2b81ffab7863afde Mon Sep 17 00:00:00 2001
From: liuhongt <hongtao.liu@intel.com>
Date: Thu, 20 May 2021 09:59:36 +0800
Subject: [PATCH] Fix ICE when lhs is NULL.

gcc/ChangeLog:
	PR target/100660
	* config/i386/i386.c (ix86_gimple_fold_builtin): Create a tmp
	variable for lhs when it doesn't exist.

gcc/testsuite/ChangeLog:
	PR target/100660
	* gcc.target/i386/pr100660.c: New test.
---
 gcc/config/i386/i386.c                   |  5 ++++-
 gcc/testsuite/gcc.target/i386/pr100660.c | 10 ++++++++++
 2 files changed, 14 insertions(+), 1 deletion(-)
 create mode 100644 gcc/testsuite/gcc.target/i386/pr100660.c

diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
index 743d8a25fe3..705257c414f 100644
--- a/gcc/config/i386/i386.c
+++ b/gcc/config/i386/i386.c
@@ -18000,7 +18000,10 @@ ix86_gimple_fold_builtin (gimple_stmt_iterator *gsi)
 	gimple_seq stmts = NULL;
 	tree cmp = gimple_build (&stmts, tcode, cmp_type, arg0, arg1);
 	gsi_insert_seq_before (gsi, stmts, GSI_SAME_STMT);
-	gimple* g = gimple_build_assign (gimple_call_lhs (stmt),
+	tree lhs = gimple_call_lhs (stmt);
+	if (!lhs)
+	  lhs = create_tmp_reg_or_ssa_name (type);
+	gimple* g = gimple_build_assign (lhs,
 					 VEC_COND_EXPR, cmp,
 					 minus_one_vec, zero_vec);
 	gimple_set_location (g, loc);
diff --git a/gcc/testsuite/gcc.target/i386/pr100660.c b/gcc/testsuite/gcc.target/i386/pr100660.c
new file mode 100644
index 00000000000..1112b742779
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/pr100660.c
@@ -0,0 +1,10 @@
+/* PR target/pr100660.  */
+/* { dg-do compile } */
+/* { dg-options "-mavx2 -O" } */
+
+typedef char v16qi __attribute__ ((vector_size (16)));
+v16qi
+f5 (v16qi a, v16qi b)
+{
+  __builtin_ia32_pcmpgtb128 (a, b);
+}
-- 
2.18.1

Re: [i386] [PATCH] Fix ICE when lhs is NULL [PR target/100660]

[Richard Biener]

On Thu, May 20, 2021 at 8:54 AM Hongtao Liu <crazylht@gmail.com> wrote:
>
> Hi:
>   In folding target-specific builtin, when lhs is NULL, create a
> temporary variable for it.
>   Bootstrapped and regtested on x86_64-linux-gnu{-m32,}

I would suggest to drop the stmt or leave it unfolded instead.
Note dropping would mean replacing it with a GIMPLE_NOP
(gimple_build_nop ()).  But creating a new unused LHS certainly
works as well.

Jakub, any preference?

Richard.

> gcc/ChangeLog:
>         PR target/100660
>         * config/i386/i386.c (ix86_gimple_fold_builtin): Create a tmp
>         variable for lhs when it doesn't exist.
>
> gcc/testsuite/ChangeLog:
>         PR target/100660
>         * gcc.target/i386/pr100660.c: New test.
>
>
>
> --
> BR,
> Hongtao

Re: [i386] [PATCH] Fix ICE when lhs is NULL [PR target/100660]

[Jakub Jelinek]

On Thu, May 20, 2021 at 10:06:36AM +0200, Richard Biener wrote:
> On Thu, May 20, 2021 at 8:54 AM Hongtao Liu <crazylht@gmail.com> wrote:
> >
> > Hi:
> >   In folding target-specific builtin, when lhs is NULL, create a
> > temporary variable for it.
> >   Bootstrapped and regtested on x86_64-linux-gnu{-m32,}
> 
> I would suggest to drop the stmt or leave it unfolded instead.
> Note dropping would mean replacing it with a GIMPLE_NOP
> (gimple_build_nop ()).  But creating a new unused LHS certainly
> works as well.
> 
> Jakub, any preference?

Yeah, nop would be my preference too.

Though, maybe even better would be make sure the builtin is const and
just punt on folding it if lhs is missing?
Then the middle-end should DCE those when lhs is gone.

	Jakub

Re: [i386] [PATCH] Fix ICE when lhs is NULL [PR target/100660]

[Hongtao Liu]

On Thu, May 20, 2021 at 4:06 PM Richard Biener
<richard.guenther@gmail.com> wrote:
>
> On Thu, May 20, 2021 at 8:54 AM Hongtao Liu <crazylht@gmail.com> wrote:
> >
> > Hi:
> >   In folding target-specific builtin, when lhs is NULL, create a
> > temporary variable for it.
> >   Bootstrapped and regtested on x86_64-linux-gnu{-m32,}
>
> I would suggest to drop the stmt or leave it unfolded instead.
Will -O0 be able to optimize the builtin away?
 Since i've deleted the corresponding expander, there would be an
error if the builtin goes directly to pass_expand.
> Note dropping would mean replacing it with a GIMPLE_NOP
> (gimple_build_nop ()).  But creating a new unused LHS certainly
> works as well.
>
> Jakub, any preference?
>
> Richard.
>
> > gcc/ChangeLog:
> >         PR target/100660
> >         * config/i386/i386.c (ix86_gimple_fold_builtin): Create a tmp
> >         variable for lhs when it doesn't exist.
> >
> > gcc/testsuite/ChangeLog:
> >         PR target/100660
> >         * gcc.target/i386/pr100660.c: New test.
> >
> >
> >
> > --
> > BR,
> > Hongtao



-- 
BR,
Hongtao

Re: [i386] [PATCH] Fix ICE when lhs is NULL [PR target/100660]

[Richard Biener]

On Thu, May 20, 2021 at 10:15 AM Hongtao Liu <crazylht@gmail.com> wrote:
>
> On Thu, May 20, 2021 at 4:06 PM Richard Biener
> <richard.guenther@gmail.com> wrote:
> >
> > On Thu, May 20, 2021 at 8:54 AM Hongtao Liu <crazylht@gmail.com> wrote:
> > >
> > > Hi:
> > >   In folding target-specific builtin, when lhs is NULL, create a
> > > temporary variable for it.
> > >   Bootstrapped and regtested on x86_64-linux-gnu{-m32,}
> >
> > I would suggest to drop the stmt or leave it unfolded instead.
> Will -O0 be able to optimize the builtin away?
>  Since i've deleted the corresponding expander, there would be an
> error if the builtin goes directly to pass_expand.

In that case replace it with a NOP, that should be safe then.

Richard.

> > Note dropping would mean replacing it with a GIMPLE_NOP
> > (gimple_build_nop ()).  But creating a new unused LHS certainly
> > works as well.
> >
> > Jakub, any preference?
> >
> > Richard.
> >
> > > gcc/ChangeLog:
> > >         PR target/100660
> > >         * config/i386/i386.c (ix86_gimple_fold_builtin): Create a tmp
> > >         variable for lhs when it doesn't exist.
> > >
> > > gcc/testsuite/ChangeLog:
> > >         PR target/100660
> > >         * gcc.target/i386/pr100660.c: New test.
> > >
> > >
> > >
> > > --
> > > BR,
> > > Hongtao
>
>
>
> --
> BR,
> Hongtao

Re: [i386] [PATCH] Fix ICE when lhs is NULL [PR target/100660]

[Hongtao Liu]

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

On Thu, May 20, 2021 at 4:30 PM Richard Biener
<richard.guenther@gmail.com> wrote:
>
> On Thu, May 20, 2021 at 10:15 AM Hongtao Liu <crazylht@gmail.com> wrote:
> >
> > On Thu, May 20, 2021 at 4:06 PM Richard Biener
> > <richard.guenther@gmail.com> wrote:
> > >
> > > On Thu, May 20, 2021 at 8:54 AM Hongtao Liu <crazylht@gmail.com> wrote:
> > > >
> > > > Hi:
> > > >   In folding target-specific builtin, when lhs is NULL, create a
> > > > temporary variable for it.
> > > >   Bootstrapped and regtested on x86_64-linux-gnu{-m32,}
> > >
> > > I would suggest to drop the stmt or leave it unfolded instead.
> > Will -O0 be able to optimize the builtin away?
> >  Since i've deleted the corresponding expander, there would be an
> > error if the builtin goes directly to pass_expand.
>
> In that case replace it with a NOP, that should be safe then.
>
update patch.
> Richard.
>
> > > Note dropping would mean replacing it with a GIMPLE_NOP
> > > (gimple_build_nop ()).  But creating a new unused LHS certainly
> > > works as well.
> > >
> > > Jakub, any preference?
> > >
> > > Richard.
> > >
> > > > gcc/ChangeLog:
> > > >         PR target/100660
> > > >         * config/i386/i386.c (ix86_gimple_fold_builtin): Create a tmp
> > > >         variable for lhs when it doesn't exist.
> > > >
> > > > gcc/testsuite/ChangeLog:
> > > >         PR target/100660
> > > >         * gcc.target/i386/pr100660.c: New test.
> > > >
> > > >
> > > >
> > > > --
> > > > BR,
> > > > Hongtao
> >
> >
> >
> > --
> > BR,
> > Hongtao



-- 
BR,
Hongtao

[-- Attachment #2: 0001-Fix-ICE-when-lhs-is-NULL.patch_v2 --]
[-- Type: application/octet-stream, Size: 2739 bytes --]

From 278c02ea21f5930cbd049d7716398e8fbfd76525 Mon Sep 17 00:00:00 2001
From: liuhongt <hongtao.liu@intel.com>
Date: Thu, 20 May 2021 09:59:36 +0800
Subject: [PATCH] Fix ICE when lhs is NULL.

gcc/ChangeLog:
	PR target/100660
	* config/i386/i386.c (ix86_gimple_fold_builtin): Replacing
	stmt with GIMPLE_NOP when lhs doesn't exist.

gcc/testsuite/ChangeLog:
	PR target/100660
	* gcc.target/i386/pr100660.c: New test.
---
 gcc/config/i386/i386.c                   | 33 +++++++++++++-----------
 gcc/testsuite/gcc.target/i386/pr100660.c | 10 +++++++
 2 files changed, 28 insertions(+), 15 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/i386/pr100660.c

diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
index 743d8a25fe3..66feb01685f 100644
--- a/gcc/config/i386/i386.c
+++ b/gcc/config/i386/i386.c
@@ -17991,21 +17991,24 @@ ix86_gimple_fold_builtin (gimple_stmt_iterator *gsi)
       gcc_assert (n_args == 2);
       arg0 = gimple_call_arg (stmt, 0);
       arg1 = gimple_call_arg (stmt, 1);
-      {
-	location_t loc = gimple_location (stmt);
-	tree type = TREE_TYPE (arg0);
-	tree zero_vec = build_zero_cst (type);
-	tree minus_one_vec = build_minus_one_cst (type);
-	tree cmp_type = truth_type_for (type);
-	gimple_seq stmts = NULL;
-	tree cmp = gimple_build (&stmts, tcode, cmp_type, arg0, arg1);
-	gsi_insert_seq_before (gsi, stmts, GSI_SAME_STMT);
-	gimple* g = gimple_build_assign (gimple_call_lhs (stmt),
-					 VEC_COND_EXPR, cmp,
-					 minus_one_vec, zero_vec);
-	gimple_set_location (g, loc);
-	gsi_replace (gsi, g, false);
-      }
+      if (gimple_call_lhs (stmt))
+	{
+	  location_t loc = gimple_location (stmt);
+	  tree type = TREE_TYPE (arg0);
+	  tree zero_vec = build_zero_cst (type);
+	  tree minus_one_vec = build_minus_one_cst (type);
+	  tree cmp_type = truth_type_for (type);
+	  gimple_seq stmts = NULL;
+	  tree cmp = gimple_build (&stmts, tcode, cmp_type, arg0, arg1);
+	  gsi_insert_seq_before (gsi, stmts, GSI_SAME_STMT);
+	  gimple* g = gimple_build_assign (gimple_call_lhs (stmt),
+					   VEC_COND_EXPR, cmp,
+					   minus_one_vec, zero_vec);
+	  gimple_set_location (g, loc);
+	  gsi_replace (gsi, g, false);
+	}
+      else
+	gsi_replace (gsi, gimple_build_nop (), false);
       return true;
 
     case IX86_BUILTIN_PSLLD:
diff --git a/gcc/testsuite/gcc.target/i386/pr100660.c b/gcc/testsuite/gcc.target/i386/pr100660.c
new file mode 100644
index 00000000000..1112b742779
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/pr100660.c
@@ -0,0 +1,10 @@
+/* PR target/pr100660.  */
+/* { dg-do compile } */
+/* { dg-options "-mavx2 -O" } */
+
+typedef char v16qi __attribute__ ((vector_size (16)));
+v16qi
+f5 (v16qi a, v16qi b)
+{
+  __builtin_ia32_pcmpgtb128 (a, b);
+}
-- 
2.18.1

Re: [i386] [PATCH] Fix ICE when lhs is NULL [PR target/100660]

[Richard Biener]

On Fri, May 21, 2021 at 4:41 AM Hongtao Liu <crazylht@gmail.com> wrote:
>
> On Thu, May 20, 2021 at 4:30 PM Richard Biener
> <richard.guenther@gmail.com> wrote:
> >
> > On Thu, May 20, 2021 at 10:15 AM Hongtao Liu <crazylht@gmail.com> wrote:
> > >
> > > On Thu, May 20, 2021 at 4:06 PM Richard Biener
> > > <richard.guenther@gmail.com> wrote:
> > > >
> > > > On Thu, May 20, 2021 at 8:54 AM Hongtao Liu <crazylht@gmail.com> wrote:
> > > > >
> > > > > Hi:
> > > > >   In folding target-specific builtin, when lhs is NULL, create a
> > > > > temporary variable for it.
> > > > >   Bootstrapped and regtested on x86_64-linux-gnu{-m32,}
> > > >
> > > > I would suggest to drop the stmt or leave it unfolded instead.
> > > Will -O0 be able to optimize the builtin away?
> > >  Since i've deleted the corresponding expander, there would be an
> > > error if the builtin goes directly to pass_expand.
> >
> > In that case replace it with a NOP, that should be safe then.
> >
> update patch.

OK.

Thanks,
Richard.

> > Richard.
> >
> > > > Note dropping would mean replacing it with a GIMPLE_NOP
> > > > (gimple_build_nop ()).  But creating a new unused LHS certainly
> > > > works as well.
> > > >
> > > > Jakub, any preference?
> > > >
> > > > Richard.
> > > >
> > > > > gcc/ChangeLog:
> > > > >         PR target/100660
> > > > >         * config/i386/i386.c (ix86_gimple_fold_builtin): Create a tmp
> > > > >         variable for lhs when it doesn't exist.
> > > > >
> > > > > gcc/testsuite/ChangeLog:
> > > > >         PR target/100660
> > > > >         * gcc.target/i386/pr100660.c: New test.
> > > > >
> > > > >
> > > > >
> > > > > --
> > > > > BR,
> > > > > Hongtao
> > >
> > >
> > >
> > > --
> > > BR,
> > > Hongtao
>
>
>
> --
> BR,
> Hongtao