Re: [PATCH] i386[stv]: Handle REG_EH_REGION note

[Hongtao Liu <crazylht@gmail.com>]

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.



-- 
BR,
Hongtao