| Message ID | CAMZc-bz7QsL0EtS5_5AU8XHmBYYjYHZmPKZOEsVQR-X4DSrpWA@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [i386] Fix ICE when lhs is NULL [PR target/100660] | expand |
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
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
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
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
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
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
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
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.