diff mbox series

i386[stv]: Handle REG_EH_REGION note

Message ID 20240314013302.994418-1-hongtao.liu@intel.com
State New
Headers show
Series i386[stv]: Handle REG_EH_REGION note | expand

Commit Message

Liu, Hongtao March 14, 2024, 1:33 a.m. UTC 
When we split
(insn 37 36 38 10 (set (reg:DI 104 [ _18 ])
        (mem:DI (reg/f:SI 98 [ CallNative_nclosure.0_1 ]) [6 MEM[(struct SQRefCounted *)CallNative_nclosure.0_1]._uiRef+0 S8 A32])) "test.C":22:42 84 {*movdi_internal}
     (expr_list:REG_EH_REGION (const_int -11 [0xfffffffffffffff5])

into

(insn 104 36 37 10 (set (subreg:V2DI (reg:DI 124) 0)
        (vec_concat:V2DI (mem:DI (reg/f:SI 98 [ CallNative_nclosure.0_1 ]) [6 MEM[(struct SQRefCounted *)CallNative_nclosure.0_1]._uiRef+0 S8 A32])
            (const_int 0 [0]))) "test.C":22:42 -1
        (nil)))
(insn 37 104 105 10 (set (subreg:V2DI (reg:DI 104 [ _18 ]) 0)
        (subreg:V2DI (reg:DI 124) 0)) "test.C":22:42 2024 {movv2di_internal}
     (expr_list:REG_EH_REGION (const_int -11 [0xfffffffffffffff5])
        (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.

Currently we only handle memory_operand, are there any other insns
need to be handled???

Bootstrapped and regtested on x86_64-pc-linux-gnu{-m32,} for trunk and gcc-13/gcc-12 release branch.
Ok for trunk and backport?

gcc/ChangeLog:

	* config/i386/i386-features.cc
	(general_scalar_chain::convert_op): Handle REG_EH_REGION note.
	(convert_scalars_to_vector): Ditto.
	* config/i386/i386-features.h (class scalar_chain): New
	memeber control_flow_insns.

gcc/testsuite/ChangeLog:

	* g++.target/i386/pr111822.C: New test.
---
 gcc/config/i386/i386-features.cc         | 48 ++++++++++++++++++++++--
 gcc/config/i386/i386-features.h          |  1 +
 gcc/testsuite/g++.target/i386/pr111822.C | 45 ++++++++++++++++++++++
 3 files changed, 90 insertions(+), 4 deletions(-)
 create mode 100644 gcc/testsuite/g++.target/i386/pr111822.C

Comments

Uros Bizjak March 14, 2024, 7:22 a.m. UTC | #1 
On Thu, Mar 14, 2024 at 2:33 AM liuhongt <hongtao.liu@intel.com> wrote:
>
> When we split
> (insn 37 36 38 10 (set (reg:DI 104 [ _18 ])
>         (mem:DI (reg/f:SI 98 [ CallNative_nclosure.0_1 ]) [6 MEM[(struct SQRefCounted *)CallNative_nclosure.0_1]._uiRef+0 S8 A32])) "test.C":22:42 84 {*movdi_internal}
>      (expr_list:REG_EH_REGION (const_int -11 [0xfffffffffffffff5])
>
> into
>
> (insn 104 36 37 10 (set (subreg:V2DI (reg:DI 124) 0)
>         (vec_concat:V2DI (mem:DI (reg/f:SI 98 [ CallNative_nclosure.0_1 ]) [6 MEM[(struct SQRefCounted *)CallNative_nclosure.0_1]._uiRef+0 S8 A32])
>             (const_int 0 [0]))) "test.C":22:42 -1
>         (nil)))
> (insn 37 104 105 10 (set (subreg:V2DI (reg:DI 104 [ _18 ]) 0)
>         (subreg:V2DI (reg:DI 124) 0)) "test.C":22:42 2024 {movv2di_internal}
>      (expr_list:REG_EH_REGION (const_int -11 [0xfffffffffffffff5])
>         (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.
>
> Currently we only handle memory_operand, are there any other insns
> need to be handled???

I think memory access is the only thing that can trap.

> Bootstrapped and regtested on x86_64-pc-linux-gnu{-m32,} for trunk and gcc-13/gcc-12 release branch.
> Ok for trunk and backport?
>
> gcc/ChangeLog:
>
>         * config/i386/i386-features.cc
>         (general_scalar_chain::convert_op): Handle REG_EH_REGION note.
>         (convert_scalars_to_vector): Ditto.
>         * config/i386/i386-features.h (class scalar_chain): New
>         memeber control_flow_insns.
>
> gcc/testsuite/ChangeLog:
>
>         * g++.target/i386/pr111822.C: New test.
> ---
>  gcc/config/i386/i386-features.cc         | 48 ++++++++++++++++++++++--
>  gcc/config/i386/i386-features.h          |  1 +
>  gcc/testsuite/g++.target/i386/pr111822.C | 45 ++++++++++++++++++++++
>  3 files changed, 90 insertions(+), 4 deletions(-)
>  create mode 100644 gcc/testsuite/g++.target/i386/pr111822.C
>
> diff --git a/gcc/config/i386/i386-features.cc b/gcc/config/i386/i386-features.cc
> index 1de2a07ed75..2ed27a9ebdd 100644
> --- a/gcc/config/i386/i386-features.cc
> +++ b/gcc/config/i386/i386-features.cc
> @@ -998,20 +998,36 @@ general_scalar_chain::convert_op (rtx *op, rtx_insn *insn)
>      }
>    else if (MEM_P (*op))
>      {
> +      rtx_insn* eh_insn, *movabs = NULL;
>        rtx tmp = gen_reg_rtx (GET_MODE (*op));
>
>        /* Handle movabs.  */
>        if (!memory_operand (*op, GET_MODE (*op)))
>         {
>           rtx tmp2 = gen_reg_rtx (GET_MODE (*op));
> +         movabs = emit_insn_before (gen_rtx_SET (tmp2, *op), insn);
>
> -         emit_insn_before (gen_rtx_SET (tmp2, *op), insn);
>           *op = tmp2;
>         }

I may be missing something, but isn't the above a dead code? We have
if (MEM_p(*op)) and then if (!memory_operand (*op, ...)).

Uros.

>
> -      emit_insn_before (gen_rtx_SET (gen_rtx_SUBREG (vmode, tmp, 0),
> -                                    gen_gpr_to_xmm_move_src (vmode, *op)),
> -                       insn);
> +      eh_insn
> +       = emit_insn_before (gen_rtx_SET (gen_rtx_SUBREG (vmode, tmp, 0),
> +                                        gen_gpr_to_xmm_move_src (vmode, *op)),
> +                           insn);
> +
> +      if (cfun->can_throw_non_call_exceptions)
> +       {
> +         /* Handle REG_EH_REGION note.  */
> +         rtx note = find_reg_note (insn, REG_EH_REGION, NULL_RTX);
> +         if (note)
> +           {
> +             if (movabs)
> +               eh_insn = movabs;
> +             control_flow_insns.safe_push (eh_insn);
> +             add_reg_note (eh_insn, REG_EH_REGION, XEXP (note, 0));
> +           }
> +       }
> +
>        *op = gen_rtx_SUBREG (vmode, tmp, 0);
>
>        if (dump_file)
> @@ -2494,6 +2510,7 @@ convert_scalars_to_vector (bool timode_p)
>  {
>    basic_block bb;
>    int converted_insns = 0;
> +  auto_vec<rtx_insn *> control_flow_insns;
>
>    bitmap_obstack_initialize (NULL);
>    const machine_mode cand_mode[3] = { SImode, DImode, TImode };
> @@ -2575,6 +2592,11 @@ convert_scalars_to_vector (bool timode_p)
>                          chain->chain_id);
>             }
>
> +         rtx_insn* iter_insn;
> +         unsigned int ii;
> +         FOR_EACH_VEC_ELT (chain->control_flow_insns, ii, iter_insn)
> +           control_flow_insns.safe_push (iter_insn);
> +
>           delete chain;
>         }
>      }
> @@ -2643,6 +2665,24 @@ convert_scalars_to_vector (bool timode_p)
>                   DECL_INCOMING_RTL (parm) = gen_rtx_SUBREG (TImode, r, 0);
>               }
>           }
> +
> +      if (!control_flow_insns.is_empty ())
> +       {
> +         free_dominance_info (CDI_DOMINATORS);
> +
> +         unsigned int i;
> +         rtx_insn* insn;
> +         FOR_EACH_VEC_ELT (control_flow_insns, i, insn)
> +           if (control_flow_insn_p (insn))
> +             {
> +               /* Split the block after insn.  There will be a fallthru
> +                  edge, which is OK so we keep it.  We have to create
> +                  the exception edges ourselves.  */
> +               bb = BLOCK_FOR_INSN (insn);
> +               split_block (bb, insn);
> +               rtl_make_eh_edge (NULL, bb, BB_END (bb));
> +             }
> +       }
>      }
>
>    return 0;
> diff --git a/gcc/config/i386/i386-features.h b/gcc/config/i386/i386-features.h
> index 8bab2d8666d..b259cf679af 100644
> --- a/gcc/config/i386/i386-features.h
> +++ b/gcc/config/i386/i386-features.h
> @@ -155,6 +155,7 @@ class scalar_chain
>    hash_map<rtx, rtx> defs_map;
>    unsigned n_sse_to_integer;
>    unsigned n_integer_to_sse;
> +  auto_vec<rtx_insn *> control_flow_insns;
>
>    bool build (bitmap candidates, unsigned insn_uid, bitmap disallowed);
>    virtual int compute_convert_gain () = 0;
> diff --git a/gcc/testsuite/g++.target/i386/pr111822.C b/gcc/testsuite/g++.target/i386/pr111822.C
> new file mode 100644
> index 00000000000..d405387b23c
> --- /dev/null
> +++ b/gcc/testsuite/g++.target/i386/pr111822.C
> @@ -0,0 +1,45 @@
> +/* PR target/111822 */
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -flive-range-shrinkage -fno-dce -fnon-call-exceptions" } */
> +
> +typedef union {
> +  int *pNativeClosure;
> +  struct SQRefCounted *pRefCounted;
> +} SQObjectValue;
> +typedef struct {
> +  SQObjectValue _unVal;
> +} SQObject;
> +typedef long SQFUNCTION(struct SQVM *);
> +struct SQVM {
> +  void CallNative();
> +};
> +struct SQRefCounted {
> +  long long _uiRef;
> +};
> +void Null();
> +struct SQObjectPtr : SQObject {
> +  SQObjectPtr() {}
> +  SQObjectPtr(int *pNativeClosure) {
> +    _unVal.pNativeClosure = pNativeClosure;
> +    _unVal.pRefCounted->_uiRef++;
> +  }
> +  ~SQObjectPtr() { --_unVal.pRefCounted->_uiRef; }
> +};
> +struct CallInfo {
> +  SQObjectPtr _closure;
> +};
> +long long _top;
> +SQFUNCTION _function;
> +int *CallNative_nclosure;
> +void SQVM::CallNative() {
> +  long long oldtop = _top;
> +  CallInfo lci;
> +  lci._closure = CallNative_nclosure;
> +  try {
> +    _function(this);
> +  } catch (...) {
> +    _top = oldtop;
> +  }
> +  while (oldtop)
> +    Null();
> +}
> --
> 2.31.1
>
Uros Bizjak March 14, 2024, 7:42 a.m. UTC | #2 
On Thu, Mar 14, 2024 at 8:32 AM Hongtao Liu <crazylht@gmail.com> wrote:
>
> On Thu, Mar 14, 2024 at 3:22 PM Uros Bizjak <ubizjak@gmail.com> wrote:
> >
> > On Thu, Mar 14, 2024 at 2:33 AM liuhongt <hongtao.liu@intel.com> wrote:
> > >
> > > When we split
> > > (insn 37 36 38 10 (set (reg:DI 104 [ _18 ])
> > >         (mem:DI (reg/f:SI 98 [ CallNative_nclosure.0_1 ]) [6 MEM[(struct SQRefCounted *)CallNative_nclosure.0_1]._uiRef+0 S8 A32])) "test.C":22:42 84 {*movdi_internal}
> > >      (expr_list:REG_EH_REGION (const_int -11 [0xfffffffffffffff5])
> > >
> > > into
> > >
> > > (insn 104 36 37 10 (set (subreg:V2DI (reg:DI 124) 0)
> > >         (vec_concat:V2DI (mem:DI (reg/f:SI 98 [ CallNative_nclosure.0_1 ]) [6 MEM[(struct SQRefCounted *)CallNative_nclosure.0_1]._uiRef+0 S8 A32])
> > >             (const_int 0 [0]))) "test.C":22:42 -1
> > >         (nil)))
> > > (insn 37 104 105 10 (set (subreg:V2DI (reg:DI 104 [ _18 ]) 0)
> > >         (subreg:V2DI (reg:DI 124) 0)) "test.C":22:42 2024 {movv2di_internal}
> > >      (expr_list:REG_EH_REGION (const_int -11 [0xfffffffffffffff5])
> > >         (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.
> > >
> > > Currently we only handle memory_operand, are there any other insns
> > > need to be handled???
> >
> > I think memory access is the only thing that can trap.
> >
> > > Bootstrapped and regtested on x86_64-pc-linux-gnu{-m32,} for trunk and gcc-13/gcc-12 release branch.
> > > Ok for trunk and backport?
> > >
> > > gcc/ChangeLog:
> > >
> > >         * config/i386/i386-features.cc
> > >         (general_scalar_chain::convert_op): Handle REG_EH_REGION note.
> > >         (convert_scalars_to_vector): Ditto.
> > >         * config/i386/i386-features.h (class scalar_chain): New
> > >         memeber control_flow_insns.
> > >
> > > gcc/testsuite/ChangeLog:
> > >
> > >         * g++.target/i386/pr111822.C: New test.
> > > ---
> > >  gcc/config/i386/i386-features.cc         | 48 ++++++++++++++++++++++--
> > >  gcc/config/i386/i386-features.h          |  1 +
> > >  gcc/testsuite/g++.target/i386/pr111822.C | 45 ++++++++++++++++++++++
> > >  3 files changed, 90 insertions(+), 4 deletions(-)
> > >  create mode 100644 gcc/testsuite/g++.target/i386/pr111822.C
> > >
> > > diff --git a/gcc/config/i386/i386-features.cc b/gcc/config/i386/i386-features.cc
> > > index 1de2a07ed75..2ed27a9ebdd 100644
> > > --- a/gcc/config/i386/i386-features.cc
> > > +++ b/gcc/config/i386/i386-features.cc
> > > @@ -998,20 +998,36 @@ general_scalar_chain::convert_op (rtx *op, rtx_insn *insn)
> > >      }
> > >    else if (MEM_P (*op))
> > >      {
> > > +      rtx_insn* eh_insn, *movabs = NULL;
> > >        rtx tmp = gen_reg_rtx (GET_MODE (*op));
> > >
> > >        /* Handle movabs.  */
> > >        if (!memory_operand (*op, GET_MODE (*op)))
> > >         {
> > >           rtx tmp2 = gen_reg_rtx (GET_MODE (*op));
> > > +         movabs = emit_insn_before (gen_rtx_SET (tmp2, *op), insn);
> > >
> > > -         emit_insn_before (gen_rtx_SET (tmp2, *op), insn);
> > >           *op = tmp2;
> > >         }
> >
> > I may be missing something, but isn't the above a dead code? We have
> > if (MEM_p(*op)) and then if (!memory_operand (*op, ...)).
> It's PR91814 #c1, memory_operand will also check invalid memory addresses.

Oh, it is even my comment ;)

Perhaps the comment should be improved to something like:

"Emit MOVABS to load from a 64-bit absolute address to a GPR."

LGTM then.

Thanks,
Uros.

> >
> > Uros.
> >
> > >
> > > -      emit_insn_before (gen_rtx_SET (gen_rtx_SUBREG (vmode, tmp, 0),
> > > -                                    gen_gpr_to_xmm_move_src (vmode, *op)),
> > > -                       insn);
> > > +      eh_insn
> > > +       = emit_insn_before (gen_rtx_SET (gen_rtx_SUBREG (vmode, tmp, 0),
> > > +                                        gen_gpr_to_xmm_move_src (vmode, *op)),
> > > +                           insn);
> > > +
> > > +      if (cfun->can_throw_non_call_exceptions)
> > > +       {
> > > +         /* Handle REG_EH_REGION note.  */
> > > +         rtx note = find_reg_note (insn, REG_EH_REGION, NULL_RTX);
> > > +         if (note)
> > > +           {
> > > +             if (movabs)
> > > +               eh_insn = movabs;
> > > +             control_flow_insns.safe_push (eh_insn);
> > > +             add_reg_note (eh_insn, REG_EH_REGION, XEXP (note, 0));
> > > +           }
> > > +       }
> > > +
> > >        *op = gen_rtx_SUBREG (vmode, tmp, 0);
> > >
> > >        if (dump_file)
> > > @@ -2494,6 +2510,7 @@ convert_scalars_to_vector (bool timode_p)
> > >  {
> > >    basic_block bb;
> > >    int converted_insns = 0;
> > > +  auto_vec<rtx_insn *> control_flow_insns;
> > >
> > >    bitmap_obstack_initialize (NULL);
> > >    const machine_mode cand_mode[3] = { SImode, DImode, TImode };
> > > @@ -2575,6 +2592,11 @@ convert_scalars_to_vector (bool timode_p)
> > >                          chain->chain_id);
> > >             }
> > >
> > > +         rtx_insn* iter_insn;
> > > +         unsigned int ii;
> > > +         FOR_EACH_VEC_ELT (chain->control_flow_insns, ii, iter_insn)
> > > +           control_flow_insns.safe_push (iter_insn);
> > > +
> > >           delete chain;
> > >         }
> > >      }
> > > @@ -2643,6 +2665,24 @@ convert_scalars_to_vector (bool timode_p)
> > >                   DECL_INCOMING_RTL (parm) = gen_rtx_SUBREG (TImode, r, 0);
> > >               }
> > >           }
> > > +
> > > +      if (!control_flow_insns.is_empty ())
> > > +       {
> > > +         free_dominance_info (CDI_DOMINATORS);
> > > +
> > > +         unsigned int i;
> > > +         rtx_insn* insn;
> > > +         FOR_EACH_VEC_ELT (control_flow_insns, i, insn)
> > > +           if (control_flow_insn_p (insn))
> > > +             {
> > > +               /* Split the block after insn.  There will be a fallthru
> > > +                  edge, which is OK so we keep it.  We have to create
> > > +                  the exception edges ourselves.  */
> > > +               bb = BLOCK_FOR_INSN (insn);
> > > +               split_block (bb, insn);
> > > +               rtl_make_eh_edge (NULL, bb, BB_END (bb));
> > > +             }
> > > +       }
> > >      }
> > >
> > >    return 0;
> > > diff --git a/gcc/config/i386/i386-features.h b/gcc/config/i386/i386-features.h
> > > index 8bab2d8666d..b259cf679af 100644
> > > --- a/gcc/config/i386/i386-features.h
> > > +++ b/gcc/config/i386/i386-features.h
> > > @@ -155,6 +155,7 @@ class scalar_chain
> > >    hash_map<rtx, rtx> defs_map;
> > >    unsigned n_sse_to_integer;
> > >    unsigned n_integer_to_sse;
> > > +  auto_vec<rtx_insn *> control_flow_insns;
> > >
> > >    bool build (bitmap candidates, unsigned insn_uid, bitmap disallowed);
> > >    virtual int compute_convert_gain () = 0;
> > > diff --git a/gcc/testsuite/g++.target/i386/pr111822.C b/gcc/testsuite/g++.target/i386/pr111822.C
> > > new file mode 100644
> > > index 00000000000..d405387b23c
> > > --- /dev/null
> > > +++ b/gcc/testsuite/g++.target/i386/pr111822.C
> > > @@ -0,0 +1,45 @@
> > > +/* PR target/111822 */
> > > +/* { dg-do compile } */
> > > +/* { dg-options "-O2 -flive-range-shrinkage -fno-dce -fnon-call-exceptions" } */
> > > +
> > > +typedef union {
> > > +  int *pNativeClosure;
> > > +  struct SQRefCounted *pRefCounted;
> > > +} SQObjectValue;
> > > +typedef struct {
> > > +  SQObjectValue _unVal;
> > > +} SQObject;
> > > +typedef long SQFUNCTION(struct SQVM *);
> > > +struct SQVM {
> > > +  void CallNative();
> > > +};
> > > +struct SQRefCounted {
> > > +  long long _uiRef;
> > > +};
> > > +void Null();
> > > +struct SQObjectPtr : SQObject {
> > > +  SQObjectPtr() {}
> > > +  SQObjectPtr(int *pNativeClosure) {
> > > +    _unVal.pNativeClosure = pNativeClosure;
> > > +    _unVal.pRefCounted->_uiRef++;
> > > +  }
> > > +  ~SQObjectPtr() { --_unVal.pRefCounted->_uiRef; }
> > > +};
> > > +struct CallInfo {
> > > +  SQObjectPtr _closure;
> > > +};
> > > +long long _top;
> > > +SQFUNCTION _function;
> > > +int *CallNative_nclosure;
> > > +void SQVM::CallNative() {
> > > +  long long oldtop = _top;
> > > +  CallInfo lci;
> > > +  lci._closure = CallNative_nclosure;
> > > +  try {
> > > +    _function(this);
> > > +  } catch (...) {
> > > +    _top = oldtop;
> > > +  }
> > > +  while (oldtop)
> > > +    Null();
> > > +}
> > > --
> > > 2.31.1
> > >
>
>
>
> --
> BR,
> Hongtao
Hongtao Liu March 14, 2024, 7:43 a.m. UTC | #3 
On Thu, Mar 14, 2024 at 3:22 PM Uros Bizjak <ubizjak@gmail.com> wrote:
>
> On Thu, Mar 14, 2024 at 2:33 AM liuhongt <hongtao.liu@intel.com> wrote:
> >
> > When we split
> > (insn 37 36 38 10 (set (reg:DI 104 [ _18 ])
> >         (mem:DI (reg/f:SI 98 [ CallNative_nclosure.0_1 ]) [6 MEM[(struct SQRefCounted *)CallNative_nclosure.0_1]._uiRef+0 S8 A32])) "test.C":22:42 84 {*movdi_internal}
> >      (expr_list:REG_EH_REGION (const_int -11 [0xfffffffffffffff5])
> >
> > into
> >
> > (insn 104 36 37 10 (set (subreg:V2DI (reg:DI 124) 0)
> >         (vec_concat:V2DI (mem:DI (reg/f:SI 98 [ CallNative_nclosure.0_1 ]) [6 MEM[(struct SQRefCounted *)CallNative_nclosure.0_1]._uiRef+0 S8 A32])
> >             (const_int 0 [0]))) "test.C":22:42 -1
> >         (nil)))
> > (insn 37 104 105 10 (set (subreg:V2DI (reg:DI 104 [ _18 ]) 0)
> >         (subreg:V2DI (reg:DI 124) 0)) "test.C":22:42 2024 {movv2di_internal}
> >      (expr_list:REG_EH_REGION (const_int -11 [0xfffffffffffffff5])
> >         (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.
> >
> > Currently we only handle memory_operand, are there any other insns
> > need to be handled???
>
> I think memory access is the only thing that can trap.
>
> > Bootstrapped and regtested on x86_64-pc-linux-gnu{-m32,} for trunk and gcc-13/gcc-12 release branch.
> > Ok for trunk and backport?
> >
> > gcc/ChangeLog:
> >
> >         * config/i386/i386-features.cc
> >         (general_scalar_chain::convert_op): Handle REG_EH_REGION note.
> >         (convert_scalars_to_vector): Ditto.
> >         * config/i386/i386-features.h (class scalar_chain): New
> >         memeber control_flow_insns.
> >
> > gcc/testsuite/ChangeLog:
> >
> >         * g++.target/i386/pr111822.C: New test.
> > ---
> >  gcc/config/i386/i386-features.cc         | 48 ++++++++++++++++++++++--
> >  gcc/config/i386/i386-features.h          |  1 +
> >  gcc/testsuite/g++.target/i386/pr111822.C | 45 ++++++++++++++++++++++
> >  3 files changed, 90 insertions(+), 4 deletions(-)
> >  create mode 100644 gcc/testsuite/g++.target/i386/pr111822.C
> >
> > diff --git a/gcc/config/i386/i386-features.cc b/gcc/config/i386/i386-features.cc
> > index 1de2a07ed75..2ed27a9ebdd 100644
> > --- a/gcc/config/i386/i386-features.cc
> > +++ b/gcc/config/i386/i386-features.cc
> > @@ -998,20 +998,36 @@ general_scalar_chain::convert_op (rtx *op, rtx_insn *insn)
> >      }
> >    else if (MEM_P (*op))
> >      {
> > +      rtx_insn* eh_insn, *movabs = NULL;
> >        rtx tmp = gen_reg_rtx (GET_MODE (*op));
> >
> >        /* Handle movabs.  */
> >        if (!memory_operand (*op, GET_MODE (*op)))
> >         {
> >           rtx tmp2 = gen_reg_rtx (GET_MODE (*op));
> > +         movabs = emit_insn_before (gen_rtx_SET (tmp2, *op), insn);
> >
> > -         emit_insn_before (gen_rtx_SET (tmp2, *op), insn);
> >           *op = tmp2;
> >         }
>
> I may be missing something, but isn't the above a dead code? We have
> if (MEM_p(*op)) and then if (!memory_operand (*op, ...)).
It's PR91814 #c1, memory_operand will also check invalid memory addresses.
>
> Uros.
>
> >
> > -      emit_insn_before (gen_rtx_SET (gen_rtx_SUBREG (vmode, tmp, 0),
> > -                                    gen_gpr_to_xmm_move_src (vmode, *op)),
> > -                       insn);
> > +      eh_insn
> > +       = emit_insn_before (gen_rtx_SET (gen_rtx_SUBREG (vmode, tmp, 0),
> > +                                        gen_gpr_to_xmm_move_src (vmode, *op)),
> > +                           insn);
> > +
> > +      if (cfun->can_throw_non_call_exceptions)
> > +       {
> > +         /* Handle REG_EH_REGION note.  */
> > +         rtx note = find_reg_note (insn, REG_EH_REGION, NULL_RTX);
> > +         if (note)
> > +           {
> > +             if (movabs)
> > +               eh_insn = movabs;
> > +             control_flow_insns.safe_push (eh_insn);
> > +             add_reg_note (eh_insn, REG_EH_REGION, XEXP (note, 0));
> > +           }
> > +       }
> > +
> >        *op = gen_rtx_SUBREG (vmode, tmp, 0);
> >
> >        if (dump_file)
> > @@ -2494,6 +2510,7 @@ convert_scalars_to_vector (bool timode_p)
> >  {
> >    basic_block bb;
> >    int converted_insns = 0;
> > +  auto_vec<rtx_insn *> control_flow_insns;
> >
> >    bitmap_obstack_initialize (NULL);
> >    const machine_mode cand_mode[3] = { SImode, DImode, TImode };
> > @@ -2575,6 +2592,11 @@ convert_scalars_to_vector (bool timode_p)
> >                          chain->chain_id);
> >             }
> >
> > +         rtx_insn* iter_insn;
> > +         unsigned int ii;
> > +         FOR_EACH_VEC_ELT (chain->control_flow_insns, ii, iter_insn)
> > +           control_flow_insns.safe_push (iter_insn);
> > +
> >           delete chain;
> >         }
> >      }
> > @@ -2643,6 +2665,24 @@ convert_scalars_to_vector (bool timode_p)
> >                   DECL_INCOMING_RTL (parm) = gen_rtx_SUBREG (TImode, r, 0);
> >               }
> >           }
> > +
> > +      if (!control_flow_insns.is_empty ())
> > +       {
> > +         free_dominance_info (CDI_DOMINATORS);
> > +
> > +         unsigned int i;
> > +         rtx_insn* insn;
> > +         FOR_EACH_VEC_ELT (control_flow_insns, i, insn)
> > +           if (control_flow_insn_p (insn))
> > +             {
> > +               /* Split the block after insn.  There will be a fallthru
> > +                  edge, which is OK so we keep it.  We have to create
> > +                  the exception edges ourselves.  */
> > +               bb = BLOCK_FOR_INSN (insn);
> > +               split_block (bb, insn);
> > +               rtl_make_eh_edge (NULL, bb, BB_END (bb));
> > +             }
> > +       }
> >      }
> >
> >    return 0;
> > diff --git a/gcc/config/i386/i386-features.h b/gcc/config/i386/i386-features.h
> > index 8bab2d8666d..b259cf679af 100644
> > --- a/gcc/config/i386/i386-features.h
> > +++ b/gcc/config/i386/i386-features.h
> > @@ -155,6 +155,7 @@ class scalar_chain
> >    hash_map<rtx, rtx> defs_map;
> >    unsigned n_sse_to_integer;
> >    unsigned n_integer_to_sse;
> > +  auto_vec<rtx_insn *> control_flow_insns;
> >
> >    bool build (bitmap candidates, unsigned insn_uid, bitmap disallowed);
> >    virtual int compute_convert_gain () = 0;
> > diff --git a/gcc/testsuite/g++.target/i386/pr111822.C b/gcc/testsuite/g++.target/i386/pr111822.C
> > new file mode 100644
> > index 00000000000..d405387b23c
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.target/i386/pr111822.C
> > @@ -0,0 +1,45 @@
> > +/* PR target/111822 */
> > +/* { dg-do compile } */
> > +/* { dg-options "-O2 -flive-range-shrinkage -fno-dce -fnon-call-exceptions" } */
> > +
> > +typedef union {
> > +  int *pNativeClosure;
> > +  struct SQRefCounted *pRefCounted;
> > +} SQObjectValue;
> > +typedef struct {
> > +  SQObjectValue _unVal;
> > +} SQObject;
> > +typedef long SQFUNCTION(struct SQVM *);
> > +struct SQVM {
> > +  void CallNative();
> > +};
> > +struct SQRefCounted {
> > +  long long _uiRef;
> > +};
> > +void Null();
> > +struct SQObjectPtr : SQObject {
> > +  SQObjectPtr() {}
> > +  SQObjectPtr(int *pNativeClosure) {
> > +    _unVal.pNativeClosure = pNativeClosure;
> > +    _unVal.pRefCounted->_uiRef++;
> > +  }
> > +  ~SQObjectPtr() { --_unVal.pRefCounted->_uiRef; }
> > +};
> > +struct CallInfo {
> > +  SQObjectPtr _closure;
> > +};
> > +long long _top;
> > +SQFUNCTION _function;
> > +int *CallNative_nclosure;
> > +void SQVM::CallNative() {
> > +  long long oldtop = _top;
> > +  CallInfo lci;
> > +  lci._closure = CallNative_nclosure;
> > +  try {
> > +    _function(this);
> > +  } catch (...) {
> > +    _top = oldtop;
> > +  }
> > +  while (oldtop)
> > +    Null();
> > +}
> > --
> > 2.31.1
> >
Uros Bizjak March 14, 2024, 2:45 p.m. UTC | #4 
On Thu, Mar 14, 2024 at 8:42 AM Uros Bizjak <ubizjak@gmail.com> wrote:
>
> On Thu, Mar 14, 2024 at 8:32 AM Hongtao Liu <crazylht@gmail.com> wrote:
> >
> > On Thu, Mar 14, 2024 at 3:22 PM Uros Bizjak <ubizjak@gmail.com> wrote:
> > >
> > > On Thu, Mar 14, 2024 at 2:33 AM liuhongt <hongtao.liu@intel.com> wrote:
> > > >
> > > > When we split
> > > > (insn 37 36 38 10 (set (reg:DI 104 [ _18 ])
> > > >         (mem:DI (reg/f:SI 98 [ CallNative_nclosure.0_1 ]) [6 MEM[(struct SQRefCounted *)CallNative_nclosure.0_1]._uiRef+0 S8 A32])) "test.C":22:42 84 {*movdi_internal}
> > > >      (expr_list:REG_EH_REGION (const_int -11 [0xfffffffffffffff5])
> > > >
> > > > into
> > > >
> > > > (insn 104 36 37 10 (set (subreg:V2DI (reg:DI 124) 0)
> > > >         (vec_concat:V2DI (mem:DI (reg/f:SI 98 [ CallNative_nclosure.0_1 ]) [6 MEM[(struct SQRefCounted *)CallNative_nclosure.0_1]._uiRef+0 S8 A32])
> > > >             (const_int 0 [0]))) "test.C":22:42 -1
> > > >         (nil)))
> > > > (insn 37 104 105 10 (set (subreg:V2DI (reg:DI 104 [ _18 ]) 0)
> > > >         (subreg:V2DI (reg:DI 124) 0)) "test.C":22:42 2024 {movv2di_internal}
> > > >      (expr_list:REG_EH_REGION (const_int -11 [0xfffffffffffffff5])
> > > >         (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.
> > > >
> > > > Currently we only handle memory_operand, are there any other insns
> > > > need to be handled???
> > >
> > > I think memory access is the only thing that can trap.
> > >
> > > > Bootstrapped and regtested on x86_64-pc-linux-gnu{-m32,} for trunk and gcc-13/gcc-12 release branch.
> > > > Ok for trunk and backport?
> > > >
> > > > gcc/ChangeLog:
> > > >
> > > >         * config/i386/i386-features.cc
> > > >         (general_scalar_chain::convert_op): Handle REG_EH_REGION note.
> > > >         (convert_scalars_to_vector): Ditto.
> > > >         * config/i386/i386-features.h (class scalar_chain): New
> > > >         memeber control_flow_insns.
> > > >
> > > > gcc/testsuite/ChangeLog:
> > > >
> > > >         * g++.target/i386/pr111822.C: New test.
> > > > ---
> > > >  gcc/config/i386/i386-features.cc         | 48 ++++++++++++++++++++++--
> > > >  gcc/config/i386/i386-features.h          |  1 +
> > > >  gcc/testsuite/g++.target/i386/pr111822.C | 45 ++++++++++++++++++++++
> > > >  3 files changed, 90 insertions(+), 4 deletions(-)
> > > >  create mode 100644 gcc/testsuite/g++.target/i386/pr111822.C
> > > >
> > > > diff --git a/gcc/config/i386/i386-features.cc b/gcc/config/i386/i386-features.cc
> > > > index 1de2a07ed75..2ed27a9ebdd 100644
> > > > --- a/gcc/config/i386/i386-features.cc
> > > > +++ b/gcc/config/i386/i386-features.cc
> > > > @@ -998,20 +998,36 @@ general_scalar_chain::convert_op (rtx *op, rtx_insn *insn)
> > > >      }
> > > >    else if (MEM_P (*op))
> > > >      {
> > > > +      rtx_insn* eh_insn, *movabs = NULL;
> > > >        rtx tmp = gen_reg_rtx (GET_MODE (*op));
> > > >
> > > >        /* Handle movabs.  */
> > > >        if (!memory_operand (*op, GET_MODE (*op)))
> > > >         {
> > > >           rtx tmp2 = gen_reg_rtx (GET_MODE (*op));
> > > > +         movabs = emit_insn_before (gen_rtx_SET (tmp2, *op), insn);
> > > >
> > > > -         emit_insn_before (gen_rtx_SET (tmp2, *op), insn);
> > > >           *op = tmp2;
> > > >         }
> > >
> > > I may be missing something, but isn't the above a dead code? We have
> > > if (MEM_p(*op)) and then if (!memory_operand (*op, ...)).
> > It's PR91814 #c1, memory_operand will also check invalid memory addresses.
>
> Oh, it is even my comment ;)
>
> Perhaps the comment should be improved to something like:
>
> "Emit MOVABS to load from a 64-bit absolute address to a GPR."
>
> LGTM then.

BTW: Do we need to also fix timode_scalar_chain::convert_op ? There we
also preload operand, so a similar fix should be applied there.

Uros.
Hongtao Liu March 15, 2024, 1:24 a.m. UTC | #5 
On Thu, Mar 14, 2024 at 10:46 PM Uros Bizjak <ubizjak@gmail.com> wrote:
>
> On Thu, Mar 14, 2024 at 8:42 AM Uros Bizjak <ubizjak@gmail.com> wrote:
> >
> > On Thu, Mar 14, 2024 at 8:32 AM Hongtao Liu <crazylht@gmail.com> wrote:
> > >
> > > On Thu, Mar 14, 2024 at 3:22 PM Uros Bizjak <ubizjak@gmail.com> wrote:
> > > >
> > > > On Thu, Mar 14, 2024 at 2:33 AM liuhongt <hongtao.liu@intel.com> wrote:
> > > > >
> > > > > When we split
> > > > > (insn 37 36 38 10 (set (reg:DI 104 [ _18 ])
> > > > >         (mem:DI (reg/f:SI 98 [ CallNative_nclosure.0_1 ]) [6 MEM[(struct SQRefCounted *)CallNative_nclosure.0_1]._uiRef+0 S8 A32])) "test.C":22:42 84 {*movdi_internal}
> > > > >      (expr_list:REG_EH_REGION (const_int -11 [0xfffffffffffffff5])
> > > > >
> > > > > into
> > > > >
> > > > > (insn 104 36 37 10 (set (subreg:V2DI (reg:DI 124) 0)
> > > > >         (vec_concat:V2DI (mem:DI (reg/f:SI 98 [ CallNative_nclosure.0_1 ]) [6 MEM[(struct SQRefCounted *)CallNative_nclosure.0_1]._uiRef+0 S8 A32])
> > > > >             (const_int 0 [0]))) "test.C":22:42 -1
> > > > >         (nil)))
> > > > > (insn 37 104 105 10 (set (subreg:V2DI (reg:DI 104 [ _18 ]) 0)
> > > > >         (subreg:V2DI (reg:DI 124) 0)) "test.C":22:42 2024 {movv2di_internal}
> > > > >      (expr_list:REG_EH_REGION (const_int -11 [0xfffffffffffffff5])
> > > > >         (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.
> > > > >
> > > > > Currently we only handle memory_operand, are there any other insns
> > > > > need to be handled???
> > > >
> > > > I think memory access is the only thing that can trap.
> > > >
> > > > > Bootstrapped and regtested on x86_64-pc-linux-gnu{-m32,} for trunk and gcc-13/gcc-12 release branch.
> > > > > Ok for trunk and backport?
> > > > >
> > > > > gcc/ChangeLog:
> > > > >
> > > > >         * config/i386/i386-features.cc
> > > > >         (general_scalar_chain::convert_op): Handle REG_EH_REGION note.
> > > > >         (convert_scalars_to_vector): Ditto.
> > > > >         * config/i386/i386-features.h (class scalar_chain): New
> > > > >         memeber control_flow_insns.
> > > > >
> > > > > gcc/testsuite/ChangeLog:
> > > > >
> > > > >         * g++.target/i386/pr111822.C: New test.
> > > > > ---
> > > > >  gcc/config/i386/i386-features.cc         | 48 ++++++++++++++++++++++--
> > > > >  gcc/config/i386/i386-features.h          |  1 +
> > > > >  gcc/testsuite/g++.target/i386/pr111822.C | 45 ++++++++++++++++++++++
> > > > >  3 files changed, 90 insertions(+), 4 deletions(-)
> > > > >  create mode 100644 gcc/testsuite/g++.target/i386/pr111822.C
> > > > >
> > > > > diff --git a/gcc/config/i386/i386-features.cc b/gcc/config/i386/i386-features.cc
> > > > > index 1de2a07ed75..2ed27a9ebdd 100644
> > > > > --- a/gcc/config/i386/i386-features.cc
> > > > > +++ b/gcc/config/i386/i386-features.cc
> > > > > @@ -998,20 +998,36 @@ general_scalar_chain::convert_op (rtx *op, rtx_insn *insn)
> > > > >      }
> > > > >    else if (MEM_P (*op))
> > > > >      {
> > > > > +      rtx_insn* eh_insn, *movabs = NULL;
> > > > >        rtx tmp = gen_reg_rtx (GET_MODE (*op));
> > > > >
> > > > >        /* Handle movabs.  */
> > > > >        if (!memory_operand (*op, GET_MODE (*op)))
> > > > >         {
> > > > >           rtx tmp2 = gen_reg_rtx (GET_MODE (*op));
> > > > > +         movabs = emit_insn_before (gen_rtx_SET (tmp2, *op), insn);
> > > > >
> > > > > -         emit_insn_before (gen_rtx_SET (tmp2, *op), insn);
> > > > >           *op = tmp2;
> > > > >         }
> > > >
> > > > I may be missing something, but isn't the above a dead code? We have
> > > > if (MEM_p(*op)) and then if (!memory_operand (*op, ...)).
> > > It's PR91814 #c1, memory_operand will also check invalid memory addresses.
> >
> > Oh, it is even my comment ;)
> >
> > Perhaps the comment should be improved to something like:
> >
> > "Emit MOVABS to load from a 64-bit absolute address to a GPR."
> >
> > LGTM then.
>
> BTW: Do we need to also fix timode_scalar_chain::convert_op ? There we
> also preload operand, so a similar fix should be applied there.
Yes, I'll make another patch. Didn't realize there are 2 of them.
>
> Uros.
diff mbox series

Patch

diff --git a/gcc/config/i386/i386-features.cc b/gcc/config/i386/i386-features.cc
index 1de2a07ed75..2ed27a9ebdd 100644
--- a/gcc/config/i386/i386-features.cc
+++ b/gcc/config/i386/i386-features.cc
@@ -998,20 +998,36 @@  general_scalar_chain::convert_op (rtx *op, rtx_insn *insn)
     }
   else if (MEM_P (*op))
     {
+      rtx_insn* eh_insn, *movabs = NULL;
       rtx tmp = gen_reg_rtx (GET_MODE (*op));
 
       /* Handle movabs.  */
       if (!memory_operand (*op, GET_MODE (*op)))
 	{
 	  rtx tmp2 = gen_reg_rtx (GET_MODE (*op));
+	  movabs = emit_insn_before (gen_rtx_SET (tmp2, *op), insn);
 
-	  emit_insn_before (gen_rtx_SET (tmp2, *op), insn);
 	  *op = tmp2;
 	}
 
-      emit_insn_before (gen_rtx_SET (gen_rtx_SUBREG (vmode, tmp, 0),
-				     gen_gpr_to_xmm_move_src (vmode, *op)),
-			insn);
+      eh_insn
+	= emit_insn_before (gen_rtx_SET (gen_rtx_SUBREG (vmode, tmp, 0),
+					 gen_gpr_to_xmm_move_src (vmode, *op)),
+			    insn);
+
+      if (cfun->can_throw_non_call_exceptions)
+	{
+	  /* Handle REG_EH_REGION note.  */
+	  rtx note = find_reg_note (insn, REG_EH_REGION, NULL_RTX);
+	  if (note)
+	    {
+	      if (movabs)
+		eh_insn = movabs;
+	      control_flow_insns.safe_push (eh_insn);
+	      add_reg_note (eh_insn, REG_EH_REGION, XEXP (note, 0));
+	    }
+	}
+
       *op = gen_rtx_SUBREG (vmode, tmp, 0);
 
       if (dump_file)
@@ -2494,6 +2510,7 @@  convert_scalars_to_vector (bool timode_p)
 {
   basic_block bb;
   int converted_insns = 0;
+  auto_vec<rtx_insn *> control_flow_insns;
 
   bitmap_obstack_initialize (NULL);
   const machine_mode cand_mode[3] = { SImode, DImode, TImode };
@@ -2575,6 +2592,11 @@  convert_scalars_to_vector (bool timode_p)
 			 chain->chain_id);
 	    }
 
+	  rtx_insn* iter_insn;
+	  unsigned int ii;
+	  FOR_EACH_VEC_ELT (chain->control_flow_insns, ii, iter_insn)
+	    control_flow_insns.safe_push (iter_insn);
+
 	  delete chain;
 	}
     }
@@ -2643,6 +2665,24 @@  convert_scalars_to_vector (bool timode_p)
 		  DECL_INCOMING_RTL (parm) = gen_rtx_SUBREG (TImode, r, 0);
 	      }
 	  }
+
+      if (!control_flow_insns.is_empty ())
+	{
+	  free_dominance_info (CDI_DOMINATORS);
+
+	  unsigned int i;
+	  rtx_insn* insn;
+	  FOR_EACH_VEC_ELT (control_flow_insns, i, insn)
+	    if (control_flow_insn_p (insn))
+	      {
+		/* Split the block after insn.  There will be a fallthru
+		   edge, which is OK so we keep it.  We have to create
+		   the exception edges ourselves.  */
+		bb = BLOCK_FOR_INSN (insn);
+		split_block (bb, insn);
+		rtl_make_eh_edge (NULL, bb, BB_END (bb));
+	      }
+	}
     }
 
   return 0;
diff --git a/gcc/config/i386/i386-features.h b/gcc/config/i386/i386-features.h
index 8bab2d8666d..b259cf679af 100644
--- a/gcc/config/i386/i386-features.h
+++ b/gcc/config/i386/i386-features.h
@@ -155,6 +155,7 @@  class scalar_chain
   hash_map<rtx, rtx> defs_map;
   unsigned n_sse_to_integer;
   unsigned n_integer_to_sse;
+  auto_vec<rtx_insn *> control_flow_insns;
 
   bool build (bitmap candidates, unsigned insn_uid, bitmap disallowed);
   virtual int compute_convert_gain () = 0;
diff --git a/gcc/testsuite/g++.target/i386/pr111822.C b/gcc/testsuite/g++.target/i386/pr111822.C
new file mode 100644
index 00000000000..d405387b23c
--- /dev/null
+++ b/gcc/testsuite/g++.target/i386/pr111822.C
@@ -0,0 +1,45 @@ 
+/* PR target/111822 */
+/* { dg-do compile } */
+/* { dg-options "-O2 -flive-range-shrinkage -fno-dce -fnon-call-exceptions" } */
+
+typedef union {
+  int *pNativeClosure;
+  struct SQRefCounted *pRefCounted;
+} SQObjectValue;
+typedef struct {
+  SQObjectValue _unVal;
+} SQObject;
+typedef long SQFUNCTION(struct SQVM *);
+struct SQVM {
+  void CallNative();
+};
+struct SQRefCounted {
+  long long _uiRef;
+};
+void Null();
+struct SQObjectPtr : SQObject {
+  SQObjectPtr() {}
+  SQObjectPtr(int *pNativeClosure) {
+    _unVal.pNativeClosure = pNativeClosure;
+    _unVal.pRefCounted->_uiRef++;
+  }
+  ~SQObjectPtr() { --_unVal.pRefCounted->_uiRef; }
+};
+struct CallInfo {
+  SQObjectPtr _closure;
+};
+long long _top;
+SQFUNCTION _function;
+int *CallNative_nclosure;
+void SQVM::CallNative() {
+  long long oldtop = _top;
+  CallInfo lci;
+  lci._closure = CallNative_nclosure;
+  try {
+    _function(this);
+  } catch (...) {
+    _top = oldtop;
+  }
+  while (oldtop)
+    Null();
+}