On Thu, Mar 14, 2019 at 1:28 AM Jakub Jelinek wrote:
>
> On Tue, Mar 12, 2019 at 09:36:32AM +0800, H.J. Lu wrote:
> > PR target/89650
> > * config/i386/i386.c (remove_partial_avx_dependency): Handle
> > REG_EH_REGION note.
> >
> > gcc/testsuite/
> >
> > PR target/89650
> > * g++.target/i386/pr89650.C: New test.
> > ---
> > gcc/config/i386/i386.c | 6 ++
> > gcc/testsuite/g++.target/i386/pr89650.C | 19 +++
> > 2 files changed, 25 insertions(+)
> > create mode 100644 gcc/testsuite/g++.target/i386/pr89650.C
> >
> > diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
> > index 896c6f33d40..b702703074c 100644
> > --- a/gcc/config/i386/i386.c
> > +++ b/gcc/config/i386/i386.c
> > @@ -2873,6 +2873,12 @@ remove_partial_avx_dependency (void)
> > /* Generate an XMM vector SET. */
> > set = gen_rtx_SET (vec, src);
> > set_insn = emit_insn_before (set, insn);
> > +
> > + /* Handle REG_EH_REGION note. */
> > + rtx note = find_reg_note (insn, REG_EH_REGION, NULL_RTX);
> > + if (note)
> > + add_reg_note (insn, REG_EH_REGION, XEXP (note, 0));
> > +
>
> How can this work? If insn has REG_EH_REGION note, you add that note
> to it once more? At minimum that should be add_reg_note (set_insn, ...).
> But perhaps better use:
> copy_reg_eh_region_note_forward (insn, set_insn, insn);
> instead?
>
> That said, with either change the testcase ICEs, as we have control flow
> insn in the middle of a bb.
>
> Tried to do:
> --- gcc/config/i386/i386.c.jj 2019-03-13 09:23:37.138538182 +0100
> +++ gcc/config/i386/i386.c 2019-03-13 18:24:07.773761751 +0100
> @@ -91,6 +91,7 @@ along with GCC; see the file COPYING3.
> #include "tree-vector-builder.h"
> #include "debug.h"
> #include "dwarf2out.h"
> +#include "cfgcleanup.h"
>
> /* This file should be included last. */
> #include "target-def.h"
> @@ -2814,6 +2815,9 @@ remove_partial_avx_dependency (void)
>bitmap_obstack_initialize (NULL);
>bitmap convert_bbs = BITMAP_ALLOC (NULL);
>
> + auto_sbitmap blocks_with_eh (last_basic_block_for_fn (cfun));
> + bitmap_clear (blocks_with_eh);
> +
>basic_block bb;
>rtx_insn *insn, *set_insn;
>rtx set;
> @@ -2873,6 +2877,15 @@ remove_partial_avx_dependency (void)
> /* Generate an XMM vector SET. */
> set = gen_rtx_SET (vec, src);
> set_insn = emit_insn_before (set, insn);
> +
> + /* Handle REG_EH_REGION note. */
> + rtx note = find_reg_note (insn, REG_EH_REGION, NULL_RTX);
> + if (note)
> + {
> + bitmap_set_bit (blocks_with_eh, bb->index);
> + add_reg_note (set_insn, REG_EH_REGION, XEXP (note, 0));
> + }
> +
> df_insn_rescan (set_insn);
>
> src = gen_rtx_SUBREG (dest_mode, vec, 0);
> @@ -2930,6 +2943,14 @@ remove_partial_avx_dependency (void)
>bitmap_obstack_release (NULL);
>BITMAP_FREE (convert_bbs);
>
> + if (!bitmap_empty_p (blocks_with_eh))
> +{
> + find_many_sub_basic_blocks (blocks_with_eh);
> +
> + /* If we've moved any REG_EH_REGION notes, also cleanup the cfg. */
> + cleanup_cfg (0);
> +}
> +
>timevar_pop (TV_MACH_DEP);
>return 0;
> }
>
> but that still ICEs.
>
> Jakub
We need to split the basic block if we create new insns, which may
throw exceptions, in the middle of the basic blocks.
Tested on AVX2 and AVX512 machines with and without
--with-arch=native
OK for trunk?
Thanks.
--
H.J.
From 713d7648c4018f2f08e179c165d0360de8ecb618 Mon Sep 17 00:00:00 2001
From: "H.J. Lu"
Date: Tue, 12 Mar 2019 08:55:24 +0800
Subject: [PATCH] i386: Handle REG_EH_REGION note
When we split:
(insn 18 17 76 2 (set (reg:SF 88 [ _19 ])
(float:SF (mem/c:SI (symbol_ref:DI ("d") [flags 0x2] ) [1 d+0 S4 A32]))) "x.ii":4:20 170 {*floatsisf2}
(expr_list:REG_EH_REGION (const_int 2 [0x2])
(nil)))
to
(insn 94 17 18 2 (set (reg:V4SF 115)
(vec_merge:V4SF (vec_duplicate:V4SF (float:SF (mem/c:SI (symbol_ref:DI ("d") [flags 0x2] ) [1 d+0 S4 A32])))
(reg:V4SF 114)
(const_int 1 [0x1]))) "x.ii":4:20 -1
(nil))
(insn 18 94 76 2 (set (reg:SF 88 [ _19 ])
(subreg:SF (reg:V4SF 115) 0)) "x.ii":4:20 112 {*movsf_internal}
(expr_list:REG_EH_REGION (const_int 2 [0x2])
(nil)))
we must copy the REG_EH_REGION note to the first insn and split the block
after the newly added insn. The REG_EH_REGION on the second insn will be
removed later since it no longer traps.
gcc/
PR target/89650
* config/i386/i386.c (remove_partial_avx_dependency): Handle
REG_EH_REGION note.
gcc/testsuite/
PR target/89650
* g++.target/i386/pr89650.C: New test.
---
gcc/config/i386/i386.c | 45 +
gcc/testsuite/g++.target/i386/pr89650.C | 19 +++
2 files changed, 64 insertions(+)
create mode 100644 gcc/testsuite/g++.target/i386/pr89650.C
