On Thu, Mar 14, 2024 at 8:42 AM Uros Bizjak <ubiz...@gmail.com> wrote:
>
> On Thu, Mar 14, 2024 at 8:32 AM Hongtao Liu <crazy...@gmail.com> wrote:
> >
> > On Thu, Mar 14, 2024 at 3:22 PM Uros Bizjak <ubiz...@gmail.com> wrote:
> > >
> > > On Thu, Mar 14, 2024 at 2:33 AM liuhongt <hongtao....@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.
