Re: [PATCH v1] RTL: Bugfix ICE after allow vector type in DSE

[Hongtao Liu <crazylht@gmail.com>]

On Mon, Feb 26, 2024 at 11:42 AM Li, Pan2 <pan2.li@intel.com> wrote:
>
> > Be Careful, It may regresses some other backend.
>
> Thanks Hongtao, how about take INNER_MODE here for regsize. Currently it will be the whole vector register when comparation.
>
> poly_uint64 regsize = REGMODE_NATURAL_SIZE (imode);
>
> Pan
>
> -----Original Message-----
> From: Hongtao Liu <crazylht@gmail.com>
> Sent: Monday, February 26, 2024 11:41 AM
> To: Li, Pan2 <pan2.li@intel.com>
> Cc: gcc-patches@gcc.gnu.org; juzhe.zhong@rivai.ai; kito.cheng@gmail.com; richard.guenther@gmail.com; Wang, Yanzhang <yanzhang.wang@intel.com>; rdapp.gcc@gmail.com
> Subject: Re: [PATCH v1] RTL: Bugfix ICE after allow vector type in DSE
>
> On Mon, Feb 26, 2024 at 11:26 AM <pan2.li@intel.com> wrote:
> >
> > From: Pan Li <pan2.li@intel.com>
> >
> > We allowed vector type for get_stored_val when read is less than or
> > equal to store in previous.  Unfortunately, we missed to adjust the
> > validate_subreg part accordingly.  For vector type, we don't need to
> > restrict the mode size is greater than the vector register size.
> >
> > Thus, for example when gen_lowpart from E_V2SFmode to E_V4QImode, it
> > will have NULL_RTX(of course ICE after that) because of the mode size
> > is less than vector register size.  That also explain that gen_lowpart
> > from E_V8SFmode to E_V16QImode is valid here.
> >
> > This patch would like to remove the the restriction for vector mode, to
> > rid of the ICE when gen_lowpart because of validate_subreg fails.
> Be Careful, It may regresses some other backend.
The related thread.
https://gcc.gnu.org/pipermail/gcc-patches/2021-August/578466.html
> >
> > The below test are passed for this patch:
> >
> > * The X86 bootstrap test.
> > * The fully riscv regression tests.
> >
> > gcc/ChangeLog:
> >
> >         * emit-rtl.cc (validate_subreg): Bypass register size check
> >         if the mode is vector.
> >
> > gcc/testsuite/ChangeLog:
> >
> >         * gcc.dg/tree-ssa/ssa-fre-44.c: Add ftree-vectorize to trigger
> >         the ICE.
> >         * gcc.target/riscv/rvv/base/bug-6.c: New test.
> >
> > Signed-off-by: Pan Li <pan2.li@intel.com>
> > ---
> >  gcc/emit-rtl.cc                               |  3 ++-
> >  gcc/testsuite/gcc.dg/tree-ssa/ssa-fre-44.c    |  2 +-
> >  .../gcc.target/riscv/rvv/base/bug-6.c         | 22 +++++++++++++++++++
> >  3 files changed, 25 insertions(+), 2 deletions(-)
> >  create mode 100644 gcc/testsuite/gcc.target/riscv/rvv/base/bug-6.c
> >
> > diff --git a/gcc/emit-rtl.cc b/gcc/emit-rtl.cc
> > index 1856fa4884f..45c6301b487 100644
> > --- a/gcc/emit-rtl.cc
> > +++ b/gcc/emit-rtl.cc
> > @@ -934,7 +934,8 @@ validate_subreg (machine_mode omode, machine_mode imode,
> >      ;
> >    /* ??? Similarly, e.g. with (subreg:DF (reg:TI)).  Though store_bit_field
> >       is the culprit here, and not the backends.  */
> > -  else if (known_ge (osize, regsize) && known_ge (isize, osize))
> > +  else if (known_ge (isize, osize) && (known_ge (osize, regsize)
> > +    || (VECTOR_MODE_P (imode) || VECTOR_MODE_P (omode))))
> >      ;
> >    /* Allow component subregs of complex and vector.  Though given the below
> >       extraction rules, it's not always clear what that means.  */
> > diff --git a/gcc/testsuite/gcc.dg/tree-ssa/ssa-fre-44.c b/gcc/testsuite/gcc.dg/tree-ssa/ssa-fre-44.c
> > index f79b4c142ae..624a00a4f32 100644
> > --- a/gcc/testsuite/gcc.dg/tree-ssa/ssa-fre-44.c
> > +++ b/gcc/testsuite/gcc.dg/tree-ssa/ssa-fre-44.c
> > @@ -1,5 +1,5 @@
> >  /* { dg-do compile } */
> > -/* { dg-options "-O -fdump-tree-fre1" } */
> > +/* { dg-options "-O -fdump-tree-fre1 -O3 -ftree-vectorize" } */
> >
> >  struct A { float x, y; };
> >  struct B { struct A u; };
> > diff --git a/gcc/testsuite/gcc.target/riscv/rvv/base/bug-6.c b/gcc/testsuite/gcc.target/riscv/rvv/base/bug-6.c
> > new file mode 100644
> > index 00000000000..5bb00b8f587
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.target/riscv/rvv/base/bug-6.c
> > @@ -0,0 +1,22 @@
> > +/* Test that we do not have ice when compile */
> > +/* { dg-do compile } */
> > +/* { dg-options "-march=rv64gcv -mabi=lp64d -O3 -ftree-vectorize" } */
> > +
> > +struct A { float x, y; };
> > +struct B { struct A u; };
> > +
> > +extern void bar (struct A *);
> > +
> > +float
> > +f3 (struct B *x, int y)
> > +{
> > +  struct A p = {1.0f, 2.0f};
> > +  struct A *q = &x[y].u;
> > +
> > +  __builtin_memcpy (&q->x, &p.x, sizeof (float));
> > +  __builtin_memcpy (&q->y, &p.y, sizeof (float));
> > +
> > +  bar (&p);
> > +
> > +  return x[y].u.x + x[y].u.y;
> > +}
> > --
> > 2.34.1
> >
>
>
> --
> BR,
> Hongtao



--
BR,
Hongtao