* [PATCH v1] RTL: Bugfix ICE after allow vector type in DSE
@ 2024-02-26 3:25 pan2.li
2024-02-26 3:41 ` Hongtao Liu
` (4 more replies)
0 siblings, 5 replies; 31+ messages in thread
From: pan2.li @ 2024-02-26 3:25 UTC (permalink / raw)
To: gcc-patches
Cc: juzhe.zhong, kito.cheng, richard.guenther, yanzhang.wang,
rdapp.gcc, Pan Li
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.
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
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v1] RTL: Bugfix ICE after allow vector type in DSE
2024-02-26 3:25 [PATCH v1] RTL: Bugfix ICE after allow vector type in DSE pan2.li
@ 2024-02-26 3:41 ` Hongtao Liu
2024-02-26 3:42 ` Li, Pan2
2024-02-26 7:38 ` Richard Biener
` (3 subsequent siblings)
4 siblings, 1 reply; 31+ messages in thread
From: Hongtao Liu @ 2024-02-26 3:41 UTC (permalink / raw)
To: pan2.li
Cc: gcc-patches, juzhe.zhong, kito.cheng, richard.guenther,
yanzhang.wang, rdapp.gcc
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 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
^ permalink raw reply [flat|nested] 31+ messages in thread
* RE: [PATCH v1] RTL: Bugfix ICE after allow vector type in DSE
2024-02-26 3:41 ` Hongtao Liu
@ 2024-02-26 3:42 ` Li, Pan2
2024-02-26 5:13 ` Hongtao Liu
0 siblings, 1 reply; 31+ messages in thread
From: Li, Pan2 @ 2024-02-26 3:42 UTC (permalink / raw)
To: Hongtao Liu
Cc: gcc-patches, juzhe.zhong, kito.cheng, richard.guenther, Wang,
Yanzhang, rdapp.gcc
> 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 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
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v1] RTL: Bugfix ICE after allow vector type in DSE
2024-02-26 3:42 ` Li, Pan2
@ 2024-02-26 5:13 ` Hongtao Liu
0 siblings, 0 replies; 31+ messages in thread
From: Hongtao Liu @ 2024-02-26 5:13 UTC (permalink / raw)
To: Li, Pan2
Cc: gcc-patches, juzhe.zhong, kito.cheng, richard.guenther, Wang,
Yanzhang, rdapp.gcc
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
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v1] RTL: Bugfix ICE after allow vector type in DSE
2024-02-26 3:25 [PATCH v1] RTL: Bugfix ICE after allow vector type in DSE pan2.li
2024-02-26 3:41 ` Hongtao Liu
@ 2024-02-26 7:38 ` Richard Biener
2024-02-26 7:41 ` Li, Pan2
2024-02-26 14:22 ` [PATCH v2] DSE: Bugfix ICE after allow vector type in get_stored_val pan2.li
` (2 subsequent siblings)
4 siblings, 1 reply; 31+ messages in thread
From: Richard Biener @ 2024-02-26 7:38 UTC (permalink / raw)
To: pan2.li; +Cc: gcc-patches, juzhe.zhong, kito.cheng, yanzhang.wang, rdapp.gcc
On Mon, Feb 26, 2024 at 4: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.
validate_subreg is a can of worms, can you try to fix the issue in DSE
by avoiding to form the subreg in the first place?
> 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
>
^ permalink raw reply [flat|nested] 31+ messages in thread
* RE: [PATCH v1] RTL: Bugfix ICE after allow vector type in DSE
2024-02-26 7:38 ` Richard Biener
@ 2024-02-26 7:41 ` Li, Pan2
0 siblings, 0 replies; 31+ messages in thread
From: Li, Pan2 @ 2024-02-26 7:41 UTC (permalink / raw)
To: Richard Biener
Cc: gcc-patches, juzhe.zhong, kito.cheng, Wang, Yanzhang, rdapp.gcc
> validate_subreg is a can of worms, can you try to fix the issue in DSE
> by avoiding to form the subreg in the first place?
Sure thing, will have a try in v2.
Pan
-----Original Message-----
From: Richard Biener <richard.guenther@gmail.com>
Sent: Monday, February 26, 2024 3:38 PM
To: Li, Pan2 <pan2.li@intel.com>
Cc: gcc-patches@gcc.gnu.org; juzhe.zhong@rivai.ai; kito.cheng@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 4: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.
validate_subreg is a can of worms, can you try to fix the issue in DSE
by avoiding to form the subreg in the first place?
> 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
>
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH v2] DSE: Bugfix ICE after allow vector type in get_stored_val
2024-02-26 3:25 [PATCH v1] RTL: Bugfix ICE after allow vector type in DSE pan2.li
2024-02-26 3:41 ` Hongtao Liu
2024-02-26 7:38 ` Richard Biener
@ 2024-02-26 14:22 ` pan2.li
2024-02-27 9:47 ` Richard Biener
2024-02-27 15:02 ` Jeff Law
2024-04-30 7:17 ` [PATCH v3] DSE: Fix " pan2.li
2024-05-03 1:51 ` [PATCH v4] " pan2.li
4 siblings, 2 replies; 31+ messages in thread
From: pan2.li @ 2024-02-26 14:22 UTC (permalink / raw)
To: gcc-patches
Cc: juzhe.zhong, kito.cheng, richard.guenther, yanzhang.wang,
rdapp.gcc, hongtao.liu, Pan Li
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. When the vector type's size is
less than vector register, it will be considered as invalid in the
validate_subreg.
Consider the validate_subreg is kind of a can with worms and we are
in stage 4. We will fix the issue from the DES side, and make sure
the subreg is valid for both the read_mode and store_mode before
perform the real gen_lowpart.
The below test are passed for this patch:
* The x86 bootstrap test.
* The x86 regression test.
* The riscv regression test.
* The aarch64 regression test.
gcc/ChangeLog:
* dse.cc (get_stored_val): Add validate_subreg check before
perform the gen_lowpart for rtl.
gcc/testsuite/ChangeLog:
* gcc.dg/tree-ssa/ssa-fre-44.c: Add compile option to trigger
the ICE.
* gcc.target/riscv/rvv/base/bug-6.c: New test.
Signed-off-by: Pan Li <pan2.li@intel.com>
---
gcc/dse.cc | 4 +++-
gcc/testsuite/gcc.dg/tree-ssa/ssa-fre-44.c | 2 +-
.../gcc.target/riscv/rvv/base/bug-6.c | 22 +++++++++++++++++++
3 files changed, 26 insertions(+), 2 deletions(-)
create mode 100644 gcc/testsuite/gcc.target/riscv/rvv/base/bug-6.c
diff --git a/gcc/dse.cc b/gcc/dse.cc
index edc7a1dfecf..1596da91da0 100644
--- a/gcc/dse.cc
+++ b/gcc/dse.cc
@@ -1946,7 +1946,9 @@ get_stored_val (store_info *store_info, machine_mode read_mode,
copy_rtx (store_info->const_rhs));
else if (VECTOR_MODE_P (read_mode) && VECTOR_MODE_P (store_mode)
&& known_le (GET_MODE_BITSIZE (read_mode), GET_MODE_BITSIZE (store_mode))
- && targetm.modes_tieable_p (read_mode, store_mode))
+ && targetm.modes_tieable_p (read_mode, store_mode)
+ && validate_subreg (read_mode, store_mode, copy_rtx (store_info->rhs),
+ subreg_lowpart_offset (read_mode, store_mode)))
read_reg = gen_lowpart (read_mode, copy_rtx (store_info->rhs));
else
read_reg = extract_low_bits (read_mode, store_mode,
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
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v2] DSE: Bugfix ICE after allow vector type in get_stored_val
2024-02-26 14:22 ` [PATCH v2] DSE: Bugfix ICE after allow vector type in get_stored_val pan2.li
@ 2024-02-27 9:47 ` Richard Biener
2024-02-27 15:02 ` Jeff Law
1 sibling, 0 replies; 31+ messages in thread
From: Richard Biener @ 2024-02-27 9:47 UTC (permalink / raw)
To: pan2.li, Richard Sandiford, Jeff Law
Cc: gcc-patches, juzhe.zhong, kito.cheng, yanzhang.wang, rdapp.gcc,
hongtao.liu
On Mon, Feb 26, 2024 at 3:22 PM <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. When the vector type's size is
> less than vector register, it will be considered as invalid in the
> validate_subreg.
>
> Consider the validate_subreg is kind of a can with worms and we are
> in stage 4. We will fix the issue from the DES side, and make sure
> the subreg is valid for both the read_mode and store_mode before
> perform the real gen_lowpart.
>
> The below test are passed for this patch:
>
> * The x86 bootstrap test.
> * The x86 regression test.
> * The riscv regression test.
> * The aarch64 regression test.
>
> gcc/ChangeLog:
>
> * dse.cc (get_stored_val): Add validate_subreg check before
> perform the gen_lowpart for rtl.
>
> gcc/testsuite/ChangeLog:
>
> * gcc.dg/tree-ssa/ssa-fre-44.c: Add compile option to trigger
> the ICE.
> * gcc.target/riscv/rvv/base/bug-6.c: New test.
>
> Signed-off-by: Pan Li <pan2.li@intel.com>
> ---
> gcc/dse.cc | 4 +++-
> gcc/testsuite/gcc.dg/tree-ssa/ssa-fre-44.c | 2 +-
> .../gcc.target/riscv/rvv/base/bug-6.c | 22 +++++++++++++++++++
> 3 files changed, 26 insertions(+), 2 deletions(-)
> create mode 100644 gcc/testsuite/gcc.target/riscv/rvv/base/bug-6.c
>
> diff --git a/gcc/dse.cc b/gcc/dse.cc
> index edc7a1dfecf..1596da91da0 100644
> --- a/gcc/dse.cc
> +++ b/gcc/dse.cc
> @@ -1946,7 +1946,9 @@ get_stored_val (store_info *store_info, machine_mode read_mode,
> copy_rtx (store_info->const_rhs));
> else if (VECTOR_MODE_P (read_mode) && VECTOR_MODE_P (store_mode)
> && known_le (GET_MODE_BITSIZE (read_mode), GET_MODE_BITSIZE (store_mode))
> - && targetm.modes_tieable_p (read_mode, store_mode))
> + && targetm.modes_tieable_p (read_mode, store_mode)
> + && validate_subreg (read_mode, store_mode, copy_rtx (store_info->rhs),
> + subreg_lowpart_offset (read_mode, store_mode)))
> read_reg = gen_lowpart (read_mode, copy_rtx (store_info->rhs));
Thanks for the 2nd try. I'll note the above uses gen_lowpart but
validate_subreg
which is sort-of a mismatch? But I'll leave this for review to people with more
knowledge in this area. Jeff? Richard?
Thanks,
Richard.
> else
> read_reg = extract_low_bits (read_mode, store_mode,
> 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
>
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v2] DSE: Bugfix ICE after allow vector type in get_stored_val
2024-02-26 14:22 ` [PATCH v2] DSE: Bugfix ICE after allow vector type in get_stored_val pan2.li
2024-02-27 9:47 ` Richard Biener
@ 2024-02-27 15:02 ` Jeff Law
2024-02-28 1:40 ` Li, Pan2
1 sibling, 1 reply; 31+ messages in thread
From: Jeff Law @ 2024-02-27 15:02 UTC (permalink / raw)
To: pan2.li, gcc-patches
Cc: juzhe.zhong, kito.cheng, richard.guenther, yanzhang.wang,
rdapp.gcc, hongtao.liu
On 2/26/24 07:22, 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. When the vector type's size is
> less than vector register, it will be considered as invalid in the
> validate_subreg.
>
> Consider the validate_subreg is kind of a can with worms and we are
> in stage 4. We will fix the issue from the DES side, and make sure
> the subreg is valid for both the read_mode and store_mode before
> perform the real gen_lowpart.
>
> The below test are passed for this patch:
>
> * The x86 bootstrap test.
> * The x86 regression test.
> * The riscv regression test.
> * The aarch64 regression test.
>
> gcc/ChangeLog:
>
> * dse.cc (get_stored_val): Add validate_subreg check before
> perform the gen_lowpart for rtl.
>
> gcc/testsuite/ChangeLog:
>
> * gcc.dg/tree-ssa/ssa-fre-44.c: Add compile option to trigger
> the ICE.
> * gcc.target/riscv/rvv/base/bug-6.c: New test.
>
> Signed-off-by: Pan Li <pan2.li@intel.com>
> ---
> gcc/dse.cc | 4 +++-
> gcc/testsuite/gcc.dg/tree-ssa/ssa-fre-44.c | 2 +-
> .../gcc.target/riscv/rvv/base/bug-6.c | 22 +++++++++++++++++++
> 3 files changed, 26 insertions(+), 2 deletions(-)
> create mode 100644 gcc/testsuite/gcc.target/riscv/rvv/base/bug-6.c
>
> diff --git a/gcc/dse.cc b/gcc/dse.cc
> index edc7a1dfecf..1596da91da0 100644
> --- a/gcc/dse.cc
> +++ b/gcc/dse.cc
> @@ -1946,7 +1946,9 @@ get_stored_val (store_info *store_info, machine_mode read_mode,
> copy_rtx (store_info->const_rhs));
> else if (VECTOR_MODE_P (read_mode) && VECTOR_MODE_P (store_mode)
> && known_le (GET_MODE_BITSIZE (read_mode), GET_MODE_BITSIZE (store_mode))
> - && targetm.modes_tieable_p (read_mode, store_mode))
> + && targetm.modes_tieable_p (read_mode, store_mode)
> + && validate_subreg (read_mode, store_mode, copy_rtx (store_info->rhs),
> + subreg_lowpart_offset (read_mode, store_mode)))
> read_reg = gen_lowpart (read_mode, copy_rtx (store_info->rhs));
> else
> read_reg = extract_low_bits (read_mode, store_mode,
So we're just changing whether or not we call gen_lowpart directly or go
through extract_low_bits, which may in turn generate subreg, call
gen_lowpart itself and a few other things.
I'm guessing that extract_low_bits is going to return NULL in this case
via this code (specifically the second test).
> if (!targetm.modes_tieable_p (src_int_mode, src_mode))
> return NULL_RTX;
> if (!targetm.modes_tieable_p (int_mode, mode))
> return NULL_RTX;
Pan, can you confirm what path we take through extract_low_bits?
One might argue that we should just call into extract_low_bits
unconditionally since it'll ultimately call gen_lowpart when it safely
can. The downside is that's a bigger change than I'd like at this stage
in our development cycle.
I wouldn't be surprised if other direct uses of gen_lowpart have similar
problems.
> 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; };
So this may compromise the original intent of this test. What I would
suggest instead is to create a new test with the dg-do & dg-options you
want with a #include "ssa-fre-44.c".
So to move forward. Let's confirm the path we take through
extract_low_bits matches expectations and fixup the testsuite change.
Jeff
^ permalink raw reply [flat|nested] 31+ messages in thread
* RE: [PATCH v2] DSE: Bugfix ICE after allow vector type in get_stored_val
2024-02-27 15:02 ` Jeff Law
@ 2024-02-28 1:40 ` Li, Pan2
2024-02-28 4:51 ` Li, Pan2
0 siblings, 1 reply; 31+ messages in thread
From: Li, Pan2 @ 2024-02-28 1:40 UTC (permalink / raw)
To: Jeff Law, gcc-patches
Cc: juzhe.zhong, kito.cheng, richard.guenther, Wang, Yanzhang,
rdapp.gcc, Liu, Hongtao
> Pan, can you confirm what path we take through extract_low_bits?
Thanks Jeff for comments, will have a try soon and keep you posted.
Pan
-----Original Message-----
From: Jeff Law <jeffreyalaw@gmail.com>
Sent: Tuesday, February 27, 2024 11:03 PM
To: Li, Pan2 <pan2.li@intel.com>; gcc-patches@gcc.gnu.org
Cc: juzhe.zhong@rivai.ai; kito.cheng@gmail.com; richard.guenther@gmail.com; Wang, Yanzhang <yanzhang.wang@intel.com>; rdapp.gcc@gmail.com; Liu, Hongtao <hongtao.liu@intel.com>
Subject: Re: [PATCH v2] DSE: Bugfix ICE after allow vector type in get_stored_val
On 2/26/24 07:22, 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. When the vector type's size is
> less than vector register, it will be considered as invalid in the
> validate_subreg.
>
> Consider the validate_subreg is kind of a can with worms and we are
> in stage 4. We will fix the issue from the DES side, and make sure
> the subreg is valid for both the read_mode and store_mode before
> perform the real gen_lowpart.
>
> The below test are passed for this patch:
>
> * The x86 bootstrap test.
> * The x86 regression test.
> * The riscv regression test.
> * The aarch64 regression test.
>
> gcc/ChangeLog:
>
> * dse.cc (get_stored_val): Add validate_subreg check before
> perform the gen_lowpart for rtl.
>
> gcc/testsuite/ChangeLog:
>
> * gcc.dg/tree-ssa/ssa-fre-44.c: Add compile option to trigger
> the ICE.
> * gcc.target/riscv/rvv/base/bug-6.c: New test.
>
> Signed-off-by: Pan Li <pan2.li@intel.com>
> ---
> gcc/dse.cc | 4 +++-
> gcc/testsuite/gcc.dg/tree-ssa/ssa-fre-44.c | 2 +-
> .../gcc.target/riscv/rvv/base/bug-6.c | 22 +++++++++++++++++++
> 3 files changed, 26 insertions(+), 2 deletions(-)
> create mode 100644 gcc/testsuite/gcc.target/riscv/rvv/base/bug-6.c
>
> diff --git a/gcc/dse.cc b/gcc/dse.cc
> index edc7a1dfecf..1596da91da0 100644
> --- a/gcc/dse.cc
> +++ b/gcc/dse.cc
> @@ -1946,7 +1946,9 @@ get_stored_val (store_info *store_info, machine_mode read_mode,
> copy_rtx (store_info->const_rhs));
> else if (VECTOR_MODE_P (read_mode) && VECTOR_MODE_P (store_mode)
> && known_le (GET_MODE_BITSIZE (read_mode), GET_MODE_BITSIZE (store_mode))
> - && targetm.modes_tieable_p (read_mode, store_mode))
> + && targetm.modes_tieable_p (read_mode, store_mode)
> + && validate_subreg (read_mode, store_mode, copy_rtx (store_info->rhs),
> + subreg_lowpart_offset (read_mode, store_mode)))
> read_reg = gen_lowpart (read_mode, copy_rtx (store_info->rhs));
> else
> read_reg = extract_low_bits (read_mode, store_mode,
So we're just changing whether or not we call gen_lowpart directly or go
through extract_low_bits, which may in turn generate subreg, call
gen_lowpart itself and a few other things.
I'm guessing that extract_low_bits is going to return NULL in this case
via this code (specifically the second test).
> if (!targetm.modes_tieable_p (src_int_mode, src_mode))
> return NULL_RTX;
> if (!targetm.modes_tieable_p (int_mode, mode))
> return NULL_RTX;
Pan, can you confirm what path we take through extract_low_bits?
One might argue that we should just call into extract_low_bits
unconditionally since it'll ultimately call gen_lowpart when it safely
can. The downside is that's a bigger change than I'd like at this stage
in our development cycle.
I wouldn't be surprised if other direct uses of gen_lowpart have similar
problems.
> 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; };
So this may compromise the original intent of this test. What I would
suggest instead is to create a new test with the dg-do & dg-options you
want with a #include "ssa-fre-44.c".
So to move forward. Let's confirm the path we take through
extract_low_bits matches expectations and fixup the testsuite change.
Jeff
^ permalink raw reply [flat|nested] 31+ messages in thread
* RE: [PATCH v2] DSE: Bugfix ICE after allow vector type in get_stored_val
2024-02-28 1:40 ` Li, Pan2
@ 2024-02-28 4:51 ` Li, Pan2
2024-02-28 17:33 ` Jeff Law
0 siblings, 1 reply; 31+ messages in thread
From: Li, Pan2 @ 2024-02-28 4:51 UTC (permalink / raw)
To: Jeff Law, gcc-patches
Cc: juzhe.zhong, kito.cheng, richard.guenther, Wang, Yanzhang,
rdapp.gcc, Liu, Hongtao
> if (!targetm.modes_tieable_p (src_int_mode, src_mode))
> return NULL_RTX;
> if (!targetm.modes_tieable_p (int_mode, mode))
> return NULL_RTX;
Yes, will return NULL_RTX for in the first if, given src_int_mode is E_DImode while src_mode is
E_V2SFmode and mode is E_V4QImode. The extract_low_bits convert the modes E_V2SFmode/E_V4QImode
to E_DImode/E_SImode in advance before tieable checking, validate_subreg and gen_lowpart.
Not sure if my understanding is correct but looks extract_low_bits cannot take care of vector modes
up to a point because vector modes are always untieable to its' int mode, and then return NULL_RTX.
Pan
-----Original Message-----
From: Li, Pan2
Sent: Wednesday, February 28, 2024 9:41 AM
To: Jeff Law <jeffreyalaw@gmail.com>; gcc-patches@gcc.gnu.org
Cc: juzhe.zhong@rivai.ai; kito.cheng@gmail.com; richard.guenther@gmail.com; Wang, Yanzhang <yanzhang.wang@intel.com>; rdapp.gcc@gmail.com; Liu, Hongtao <Hongtao.Liu@intel.com>
Subject: RE: [PATCH v2] DSE: Bugfix ICE after allow vector type in get_stored_val
> Pan, can you confirm what path we take through extract_low_bits?
Thanks Jeff for comments, will have a try soon and keep you posted.
Pan
-----Original Message-----
From: Jeff Law <jeffreyalaw@gmail.com>
Sent: Tuesday, February 27, 2024 11:03 PM
To: Li, Pan2 <pan2.li@intel.com>; gcc-patches@gcc.gnu.org
Cc: juzhe.zhong@rivai.ai; kito.cheng@gmail.com; richard.guenther@gmail.com; Wang, Yanzhang <yanzhang.wang@intel.com>; rdapp.gcc@gmail.com; Liu, Hongtao <hongtao.liu@intel.com>
Subject: Re: [PATCH v2] DSE: Bugfix ICE after allow vector type in get_stored_val
On 2/26/24 07:22, 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. When the vector type's size is
> less than vector register, it will be considered as invalid in the
> validate_subreg.
>
> Consider the validate_subreg is kind of a can with worms and we are
> in stage 4. We will fix the issue from the DES side, and make sure
> the subreg is valid for both the read_mode and store_mode before
> perform the real gen_lowpart.
>
> The below test are passed for this patch:
>
> * The x86 bootstrap test.
> * The x86 regression test.
> * The riscv regression test.
> * The aarch64 regression test.
>
> gcc/ChangeLog:
>
> * dse.cc (get_stored_val): Add validate_subreg check before
> perform the gen_lowpart for rtl.
>
> gcc/testsuite/ChangeLog:
>
> * gcc.dg/tree-ssa/ssa-fre-44.c: Add compile option to trigger
> the ICE.
> * gcc.target/riscv/rvv/base/bug-6.c: New test.
>
> Signed-off-by: Pan Li <pan2.li@intel.com>
> ---
> gcc/dse.cc | 4 +++-
> gcc/testsuite/gcc.dg/tree-ssa/ssa-fre-44.c | 2 +-
> .../gcc.target/riscv/rvv/base/bug-6.c | 22 +++++++++++++++++++
> 3 files changed, 26 insertions(+), 2 deletions(-)
> create mode 100644 gcc/testsuite/gcc.target/riscv/rvv/base/bug-6.c
>
> diff --git a/gcc/dse.cc b/gcc/dse.cc
> index edc7a1dfecf..1596da91da0 100644
> --- a/gcc/dse.cc
> +++ b/gcc/dse.cc
> @@ -1946,7 +1946,9 @@ get_stored_val (store_info *store_info, machine_mode read_mode,
> copy_rtx (store_info->const_rhs));
> else if (VECTOR_MODE_P (read_mode) && VECTOR_MODE_P (store_mode)
> && known_le (GET_MODE_BITSIZE (read_mode), GET_MODE_BITSIZE (store_mode))
> - && targetm.modes_tieable_p (read_mode, store_mode))
> + && targetm.modes_tieable_p (read_mode, store_mode)
> + && validate_subreg (read_mode, store_mode, copy_rtx (store_info->rhs),
> + subreg_lowpart_offset (read_mode, store_mode)))
> read_reg = gen_lowpart (read_mode, copy_rtx (store_info->rhs));
> else
> read_reg = extract_low_bits (read_mode, store_mode,
So we're just changing whether or not we call gen_lowpart directly or go
through extract_low_bits, which may in turn generate subreg, call
gen_lowpart itself and a few other things.
I'm guessing that extract_low_bits is going to return NULL in this case
via this code (specifically the second test).
> if (!targetm.modes_tieable_p (src_int_mode, src_mode))
> return NULL_RTX;
> if (!targetm.modes_tieable_p (int_mode, mode))
> return NULL_RTX;
Pan, can you confirm what path we take through extract_low_bits?
One might argue that we should just call into extract_low_bits
unconditionally since it'll ultimately call gen_lowpart when it safely
can. The downside is that's a bigger change than I'd like at this stage
in our development cycle.
I wouldn't be surprised if other direct uses of gen_lowpart have similar
problems.
> 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; };
So this may compromise the original intent of this test. What I would
suggest instead is to create a new test with the dg-do & dg-options you
want with a #include "ssa-fre-44.c".
So to move forward. Let's confirm the path we take through
extract_low_bits matches expectations and fixup the testsuite change.
Jeff
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v2] DSE: Bugfix ICE after allow vector type in get_stored_val
2024-02-28 4:51 ` Li, Pan2
@ 2024-02-28 17:33 ` Jeff Law
2024-02-29 1:38 ` Li, Pan2
0 siblings, 1 reply; 31+ messages in thread
From: Jeff Law @ 2024-02-28 17:33 UTC (permalink / raw)
To: Li, Pan2, gcc-patches
Cc: juzhe.zhong, kito.cheng, richard.guenther, Wang, Yanzhang,
rdapp.gcc, Liu, Hongtao
On 2/27/24 21:51, Li, Pan2 wrote:
>> if (!targetm.modes_tieable_p (src_int_mode, src_mode))
>> return NULL_RTX;
>> if (!targetm.modes_tieable_p (int_mode, mode))
>> return NULL_RTX;
>
> Yes, will return NULL_RTX for in the first if, given src_int_mode is E_DImode while src_mode is
> E_V2SFmode and mode is E_V4QImode. The extract_low_bits convert the modes E_V2SFmode/E_V4QImode
> to E_DImode/E_SImode in advance before tieable checking, validate_subreg and gen_lowpart.
>
> Not sure if my understanding is correct but looks extract_low_bits cannot take care of vector modes
> up to a point because vector modes are always untieable to its' int mode, and then return NULL_RTX.
Well, the code tries to turn the vector mode into a suitable integer
mode via int_mode_for_mode. That takes a mode, including vector modes
and tries to find an integer mode of the exact same size.
So it's going to check if V2SF can be tied to DI and V4QI with SI. I
suspect those are going to fail for RISC-V as those aren't tieable.
Jeff
^ permalink raw reply [flat|nested] 31+ messages in thread
* RE: [PATCH v2] DSE: Bugfix ICE after allow vector type in get_stored_val
2024-02-28 17:33 ` Jeff Law
@ 2024-02-29 1:38 ` Li, Pan2
2024-02-29 13:28 ` Robin Dapp
0 siblings, 1 reply; 31+ messages in thread
From: Li, Pan2 @ 2024-02-29 1:38 UTC (permalink / raw)
To: Jeff Law, gcc-patches
Cc: juzhe.zhong, kito.cheng, richard.guenther, Wang, Yanzhang,
rdapp.gcc, Liu, Hongtao
> So it's going to check if V2SF can be tied to DI and V4QI with SI. I
> suspect those are going to fail for RISC-V as those aren't tieable.
Yes, you are right. Different REG_CLASS are not allowed to be tieable in RISC-V.
static bool
riscv_modes_tieable_p (machine_mode mode1, machine_mode mode2)
{
/* We don't allow different REG_CLASS modes tieable since it
will cause ICE in register allocation (RA).
E.g. V2SI and DI are not tieable. */
if (riscv_v_ext_mode_p (mode1) != riscv_v_ext_mode_p (mode2))
return false;
return (mode1 == mode2
|| !(GET_MODE_CLASS (mode1) == MODE_FLOAT
&& GET_MODE_CLASS (mode2) == MODE_FLOAT));
}
Pan
-----Original Message-----
From: Jeff Law <jeffreyalaw@gmail.com>
Sent: Thursday, February 29, 2024 1:33 AM
To: Li, Pan2 <pan2.li@intel.com>; gcc-patches@gcc.gnu.org
Cc: juzhe.zhong@rivai.ai; kito.cheng@gmail.com; richard.guenther@gmail.com; Wang, Yanzhang <yanzhang.wang@intel.com>; rdapp.gcc@gmail.com; Liu, Hongtao <hongtao.liu@intel.com>
Subject: Re: [PATCH v2] DSE: Bugfix ICE after allow vector type in get_stored_val
On 2/27/24 21:51, Li, Pan2 wrote:
>> if (!targetm.modes_tieable_p (src_int_mode, src_mode))
>> return NULL_RTX;
>> if (!targetm.modes_tieable_p (int_mode, mode))
>> return NULL_RTX;
>
> Yes, will return NULL_RTX for in the first if, given src_int_mode is E_DImode while src_mode is
> E_V2SFmode and mode is E_V4QImode. The extract_low_bits convert the modes E_V2SFmode/E_V4QImode
> to E_DImode/E_SImode in advance before tieable checking, validate_subreg and gen_lowpart.
>
> Not sure if my understanding is correct but looks extract_low_bits cannot take care of vector modes
> up to a point because vector modes are always untieable to its' int mode, and then return NULL_RTX.
Well, the code tries to turn the vector mode into a suitable integer
mode via int_mode_for_mode. That takes a mode, including vector modes
and tries to find an integer mode of the exact same size.
So it's going to check if V2SF can be tied to DI and V4QI with SI. I
suspect those are going to fail for RISC-V as those aren't tieable.
Jeff
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v2] DSE: Bugfix ICE after allow vector type in get_stored_val
2024-02-29 1:38 ` Li, Pan2
@ 2024-02-29 13:28 ` Robin Dapp
2024-03-02 1:04 ` Li, Pan2
2024-03-03 22:46 ` Jeff Law
0 siblings, 2 replies; 31+ messages in thread
From: Robin Dapp @ 2024-02-29 13:28 UTC (permalink / raw)
To: Li, Pan2, Jeff Law, gcc-patches
Cc: rdapp.gcc, juzhe.zhong, kito.cheng, richard.guenther, Wang,
Yanzhang, Liu, Hongtao
On 2/29/24 02:38, Li, Pan2 wrote:
>> So it's going to check if V2SF can be tied to DI and V4QI with SI. I
>> suspect those are going to fail for RISC-V as those aren't tieable.
>
> Yes, you are right. Different REG_CLASS are not allowed to be tieable in RISC-V.
>
> static bool
> riscv_modes_tieable_p (machine_mode mode1, machine_mode mode2)
> {
> /* We don't allow different REG_CLASS modes tieable since it
> will cause ICE in register allocation (RA).
> E.g. V2SI and DI are not tieable. */
> if (riscv_v_ext_mode_p (mode1) != riscv_v_ext_mode_p (mode2))
> return false;
> return (mode1 == mode2
> || !(GET_MODE_CLASS (mode1) == MODE_FLOAT
> && GET_MODE_CLASS (mode2) == MODE_FLOAT));
> }
Yes, but what we set tieable is e.g. V4QI and V2SF.
I suggested a target band-aid before:
diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
index 799d7919a4a..982ca1a4250 100644
--- a/gcc/config/riscv/riscv.cc
+++ b/gcc/config/riscv/riscv.cc
@@ -8208,6 +8208,11 @@ riscv_modes_tieable_p (machine_mode mode1, machine_mode mode2)
E.g. V2SI and DI are not tieable. */
if (riscv_v_ext_mode_p (mode1) != riscv_v_ext_mode_p (mode2))
return false;
+ if (GET_MODE_CLASS (GET_MODE_INNER (mode1)) == MODE_INT
+ && GET_MODE_CLASS (GET_MODE_INNER (mode2)) == MODE_FLOAT
+ && GET_MODE_SIZE (GET_MODE_INNER (mode1))
+ != GET_MODE_SIZE (GET_MODE_INNER (mode2)))
+ return false;
return (mode1 == mode2
|| !(GET_MODE_CLASS (mode1) == MODE_FLOAT
&& GET_MODE_CLASS (mode2) == MODE_FLOAT));
but I don't like that as it just works around something
that I didn't even understand fully...
Regards
Robin
^ permalink raw reply [flat|nested] 31+ messages in thread
* RE: [PATCH v2] DSE: Bugfix ICE after allow vector type in get_stored_val
2024-02-29 13:28 ` Robin Dapp
@ 2024-03-02 1:04 ` Li, Pan2
2024-03-03 22:46 ` Jeff Law
1 sibling, 0 replies; 31+ messages in thread
From: Li, Pan2 @ 2024-03-02 1:04 UTC (permalink / raw)
To: Robin Dapp, Jeff Law, gcc-patches
Cc: juzhe.zhong, kito.cheng, richard.guenther, Wang, Yanzhang, Liu, Hongtao
Yeah, talking about this with robin offline for this fix.
> Yes, but what we set tieable is e.g. V4QI and V2SF.
That comes from different code lines.
Jeff would like to learn more about extract_low_bits, it will first convert to int_mode and then call the tieable_p.
And I bet the V4QI and V2SF comes from the if condition for gen_lowpart.
--- a/gcc/dse.cc
+++ b/gcc/dse.cc
@@ -1946,7 +1946,9 @@ get_stored_val (store_info *store_info, machine_mode read_mode,
copy_rtx (store_info->const_rhs));
else if (VECTOR_MODE_P (read_mode) && VECTOR_MODE_P (store_mode)
&& known_le (GET_MODE_BITSIZE (read_mode), GET_MODE_BITSIZE (store_mode))
- && targetm.modes_tieable_p (read_mode, store_mode)) // <= V4QI and V2SF here.
+ && targetm.modes_tieable_p (read_mode, store_mode)
+ && validate_subreg (read_mode, store_mode, copy_rtx (store_info->rhs),
+ subreg_lowpart_offset (read_mode, store_mode)))
read_reg = gen_lowpart (read_mode, copy_rtx (store_info->rhs));
else
read_reg = extract_low_bits (read_mode, store_mode,
Pan
-----Original Message-----
From: Robin Dapp <rdapp.gcc@gmail.com>
Sent: Thursday, February 29, 2024 9:29 PM
To: Li, Pan2 <pan2.li@intel.com>; Jeff Law <jeffreyalaw@gmail.com>; gcc-patches@gcc.gnu.org
Cc: rdapp.gcc@gmail.com; juzhe.zhong@rivai.ai; kito.cheng@gmail.com; richard.guenther@gmail.com; Wang, Yanzhang <yanzhang.wang@intel.com>; Liu, Hongtao <hongtao.liu@intel.com>
Subject: Re: [PATCH v2] DSE: Bugfix ICE after allow vector type in get_stored_val
On 2/29/24 02:38, Li, Pan2 wrote:
>> So it's going to check if V2SF can be tied to DI and V4QI with SI. I
>> suspect those are going to fail for RISC-V as those aren't tieable.
>
> Yes, you are right. Different REG_CLASS are not allowed to be tieable in RISC-V.
>
> static bool
> riscv_modes_tieable_p (machine_mode mode1, machine_mode mode2)
> {
> /* We don't allow different REG_CLASS modes tieable since it
> will cause ICE in register allocation (RA).
> E.g. V2SI and DI are not tieable. */
> if (riscv_v_ext_mode_p (mode1) != riscv_v_ext_mode_p (mode2))
> return false;
> return (mode1 == mode2
> || !(GET_MODE_CLASS (mode1) == MODE_FLOAT
> && GET_MODE_CLASS (mode2) == MODE_FLOAT));
> }
Yes, but what we set tieable is e.g. V4QI and V2SF.
I suggested a target band-aid before:
diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
index 799d7919a4a..982ca1a4250 100644
--- a/gcc/config/riscv/riscv.cc
+++ b/gcc/config/riscv/riscv.cc
@@ -8208,6 +8208,11 @@ riscv_modes_tieable_p (machine_mode mode1, machine_mode mode2)
E.g. V2SI and DI are not tieable. */
if (riscv_v_ext_mode_p (mode1) != riscv_v_ext_mode_p (mode2))
return false;
+ if (GET_MODE_CLASS (GET_MODE_INNER (mode1)) == MODE_INT
+ && GET_MODE_CLASS (GET_MODE_INNER (mode2)) == MODE_FLOAT
+ && GET_MODE_SIZE (GET_MODE_INNER (mode1))
+ != GET_MODE_SIZE (GET_MODE_INNER (mode2)))
+ return false;
return (mode1 == mode2
|| !(GET_MODE_CLASS (mode1) == MODE_FLOAT
&& GET_MODE_CLASS (mode2) == MODE_FLOAT));
but I don't like that as it just works around something
that I didn't even understand fully...
Regards
Robin
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v2] DSE: Bugfix ICE after allow vector type in get_stored_val
2024-02-29 13:28 ` Robin Dapp
2024-03-02 1:04 ` Li, Pan2
@ 2024-03-03 22:46 ` Jeff Law
2024-03-05 6:22 ` Li, Pan2
1 sibling, 1 reply; 31+ messages in thread
From: Jeff Law @ 2024-03-03 22:46 UTC (permalink / raw)
To: Robin Dapp, Li, Pan2, gcc-patches
Cc: juzhe.zhong, kito.cheng, richard.guenther, Wang, Yanzhang, Liu, Hongtao
On 2/29/24 06:28, Robin Dapp wrote:
> On 2/29/24 02:38, Li, Pan2 wrote:
>>> So it's going to check if V2SF can be tied to DI and V4QI with SI. I
>>> suspect those are going to fail for RISC-V as those aren't tieable.
>>
>> Yes, you are right. Different REG_CLASS are not allowed to be tieable in RISC-V.
>>
>> static bool
>> riscv_modes_tieable_p (machine_mode mode1, machine_mode mode2)
>> {
>> /* We don't allow different REG_CLASS modes tieable since it
>> will cause ICE in register allocation (RA).
>> E.g. V2SI and DI are not tieable. */
>> if (riscv_v_ext_mode_p (mode1) != riscv_v_ext_mode_p (mode2))
>> return false;
>> return (mode1 == mode2
>> || !(GET_MODE_CLASS (mode1) == MODE_FLOAT
>> && GET_MODE_CLASS (mode2) == MODE_FLOAT));
>> }
>
> Yes, but what we set tieable is e.g. V4QI and V2SF.
But in the case of a vector modes, we can usually reinterpret the
underlying bits in whatever mode we want and do any of the usual
operations on those bits.
In my mind that's fundamentally different than the int vs fp case. If
we have an integer value in an FP register, we can't really operate on
the value in any sensible way without first copying it over to the
integer register file and vice-versa.
Jeff
^ permalink raw reply [flat|nested] 31+ messages in thread
* RE: [PATCH v2] DSE: Bugfix ICE after allow vector type in get_stored_val
2024-03-03 22:46 ` Jeff Law
@ 2024-03-05 6:22 ` Li, Pan2
2024-03-12 2:08 ` Li, Pan2
2024-03-22 18:53 ` Jeff Law
0 siblings, 2 replies; 31+ messages in thread
From: Li, Pan2 @ 2024-03-05 6:22 UTC (permalink / raw)
To: Jeff Law, Robin Dapp, gcc-patches
Cc: juzhe.zhong, kito.cheng, richard.guenther, Wang, Yanzhang, Liu, Hongtao
Thanks Jeff for comments.
> But in the case of a vector modes, we can usually reinterpret the
> underlying bits in whatever mode we want and do any of the usual
> operations on those bits.
Yes, I think that is why we can allow vector mode in get_stored_val if my understanding is correct.
And then the different modes will return by gen_low_part. Unfortunately, there are some modes
(less than a vector bit size like V2SF, V2QI for vlen=128) are considered as invalid by validate_subreg,
and return NULL_RTX result in the final ICE.
Thus, consider stage 4 I wonder if this is a acceptable fix, aka find some where to filter-out the invalid
modes before goes to gen_low_part.
Pan
-----Original Message-----
From: Jeff Law <jeffreyalaw@gmail.com>
Sent: Monday, March 4, 2024 6:47 AM
To: Robin Dapp <rdapp.gcc@gmail.com>; Li, Pan2 <pan2.li@intel.com>; gcc-patches@gcc.gnu.org
Cc: juzhe.zhong@rivai.ai; kito.cheng@gmail.com; richard.guenther@gmail.com; Wang, Yanzhang <yanzhang.wang@intel.com>; Liu, Hongtao <hongtao.liu@intel.com>
Subject: Re: [PATCH v2] DSE: Bugfix ICE after allow vector type in get_stored_val
On 2/29/24 06:28, Robin Dapp wrote:
> On 2/29/24 02:38, Li, Pan2 wrote:
>>> So it's going to check if V2SF can be tied to DI and V4QI with SI. I
>>> suspect those are going to fail for RISC-V as those aren't tieable.
>>
>> Yes, you are right. Different REG_CLASS are not allowed to be tieable in RISC-V.
>>
>> static bool
>> riscv_modes_tieable_p (machine_mode mode1, machine_mode mode2)
>> {
>> /* We don't allow different REG_CLASS modes tieable since it
>> will cause ICE in register allocation (RA).
>> E.g. V2SI and DI are not tieable. */
>> if (riscv_v_ext_mode_p (mode1) != riscv_v_ext_mode_p (mode2))
>> return false;
>> return (mode1 == mode2
>> || !(GET_MODE_CLASS (mode1) == MODE_FLOAT
>> && GET_MODE_CLASS (mode2) == MODE_FLOAT));
>> }
>
> Yes, but what we set tieable is e.g. V4QI and V2SF.
But in the case of a vector modes, we can usually reinterpret the
underlying bits in whatever mode we want and do any of the usual
operations on those bits.
In my mind that's fundamentally different than the int vs fp case. If
we have an integer value in an FP register, we can't really operate on
the value in any sensible way without first copying it over to the
integer register file and vice-versa.
Jeff
^ permalink raw reply [flat|nested] 31+ messages in thread
* RE: [PATCH v2] DSE: Bugfix ICE after allow vector type in get_stored_val
2024-03-05 6:22 ` Li, Pan2
@ 2024-03-12 2:08 ` Li, Pan2
2024-03-22 1:15 ` Li, Pan2
2024-03-22 18:53 ` Jeff Law
1 sibling, 1 reply; 31+ messages in thread
From: Li, Pan2 @ 2024-03-12 2:08 UTC (permalink / raw)
To: Jeff Law, Robin Dapp, gcc-patches
Cc: juzhe.zhong, kito.cheng, richard.guenther, Wang, Yanzhang, Liu, Hongtao
Hi Jeff,
Is there any suggestion(s) for how to fix this ICE in the reasonable approach? Thanks a lot.
Pan
-----Original Message-----
From: Li, Pan2
Sent: Tuesday, March 5, 2024 2:23 PM
To: Jeff Law <jeffreyalaw@gmail.com>; Robin Dapp <rdapp.gcc@gmail.com>; gcc-patches@gcc.gnu.org
Cc: juzhe.zhong@rivai.ai; kito.cheng@gmail.com; richard.guenther@gmail.com; Wang, Yanzhang <yanzhang.wang@intel.com>; Liu, Hongtao <Hongtao.Liu@intel.com>
Subject: RE: [PATCH v2] DSE: Bugfix ICE after allow vector type in get_stored_val
Thanks Jeff for comments.
> But in the case of a vector modes, we can usually reinterpret the
> underlying bits in whatever mode we want and do any of the usual
> operations on those bits.
Yes, I think that is why we can allow vector mode in get_stored_val if my understanding is correct.
And then the different modes will return by gen_low_part. Unfortunately, there are some modes
(less than a vector bit size like V2SF, V2QI for vlen=128) are considered as invalid by validate_subreg,
and return NULL_RTX result in the final ICE.
Thus, consider stage 4 I wonder if this is a acceptable fix, aka find some where to filter-out the invalid
modes before goes to gen_low_part.
Pan
-----Original Message-----
From: Jeff Law <jeffreyalaw@gmail.com>
Sent: Monday, March 4, 2024 6:47 AM
To: Robin Dapp <rdapp.gcc@gmail.com>; Li, Pan2 <pan2.li@intel.com>; gcc-patches@gcc.gnu.org
Cc: juzhe.zhong@rivai.ai; kito.cheng@gmail.com; richard.guenther@gmail.com; Wang, Yanzhang <yanzhang.wang@intel.com>; Liu, Hongtao <hongtao.liu@intel.com>
Subject: Re: [PATCH v2] DSE: Bugfix ICE after allow vector type in get_stored_val
On 2/29/24 06:28, Robin Dapp wrote:
> On 2/29/24 02:38, Li, Pan2 wrote:
>>> So it's going to check if V2SF can be tied to DI and V4QI with SI. I
>>> suspect those are going to fail for RISC-V as those aren't tieable.
>>
>> Yes, you are right. Different REG_CLASS are not allowed to be tieable in RISC-V.
>>
>> static bool
>> riscv_modes_tieable_p (machine_mode mode1, machine_mode mode2)
>> {
>> /* We don't allow different REG_CLASS modes tieable since it
>> will cause ICE in register allocation (RA).
>> E.g. V2SI and DI are not tieable. */
>> if (riscv_v_ext_mode_p (mode1) != riscv_v_ext_mode_p (mode2))
>> return false;
>> return (mode1 == mode2
>> || !(GET_MODE_CLASS (mode1) == MODE_FLOAT
>> && GET_MODE_CLASS (mode2) == MODE_FLOAT));
>> }
>
> Yes, but what we set tieable is e.g. V4QI and V2SF.
But in the case of a vector modes, we can usually reinterpret the
underlying bits in whatever mode we want and do any of the usual
operations on those bits.
In my mind that's fundamentally different than the int vs fp case. If
we have an integer value in an FP register, we can't really operate on
the value in any sensible way without first copying it over to the
integer register file and vice-versa.
Jeff
^ permalink raw reply [flat|nested] 31+ messages in thread
* RE: [PATCH v2] DSE: Bugfix ICE after allow vector type in get_stored_val
2024-03-12 2:08 ` Li, Pan2
@ 2024-03-22 1:15 ` Li, Pan2
0 siblings, 0 replies; 31+ messages in thread
From: Li, Pan2 @ 2024-03-22 1:15 UTC (permalink / raw)
To: Jeff Law, gcc-patches
Cc: juzhe.zhong, kito.cheng, richard.guenther, Wang, Yanzhang, Liu, Hongtao
Sorry for disturbing, kindly ping for this ICE.
Pan
-----Original Message-----
From: Li, Pan2 <pan2.li@intel.com>
Sent: Tuesday, March 12, 2024 10:09 AM
To: Jeff Law <jeffreyalaw@gmail.com>; Robin Dapp <rdapp.gcc@gmail.com>; gcc-patches@gcc.gnu.org
Cc: juzhe.zhong@rivai.ai; kito.cheng@gmail.com; richard.guenther@gmail.com; Wang, Yanzhang <yanzhang.wang@intel.com>; Liu, Hongtao <hongtao.liu@intel.com>
Subject: RE: [PATCH v2] DSE: Bugfix ICE after allow vector type in get_stored_val
Hi Jeff,
Is there any suggestion(s) for how to fix this ICE in the reasonable approach? Thanks a lot.
Pan
-----Original Message-----
From: Li, Pan2
Sent: Tuesday, March 5, 2024 2:23 PM
To: Jeff Law <jeffreyalaw@gmail.com>; Robin Dapp <rdapp.gcc@gmail.com>; gcc-patches@gcc.gnu.org
Cc: juzhe.zhong@rivai.ai; kito.cheng@gmail.com; richard.guenther@gmail.com; Wang, Yanzhang <yanzhang.wang@intel.com>; Liu, Hongtao <Hongtao.Liu@intel.com>
Subject: RE: [PATCH v2] DSE: Bugfix ICE after allow vector type in get_stored_val
Thanks Jeff for comments.
> But in the case of a vector modes, we can usually reinterpret the
> underlying bits in whatever mode we want and do any of the usual
> operations on those bits.
Yes, I think that is why we can allow vector mode in get_stored_val if my understanding is correct.
And then the different modes will return by gen_low_part. Unfortunately, there are some modes
(less than a vector bit size like V2SF, V2QI for vlen=128) are considered as invalid by validate_subreg,
and return NULL_RTX result in the final ICE.
Thus, consider stage 4 I wonder if this is a acceptable fix, aka find some where to filter-out the invalid
modes before goes to gen_low_part.
Pan
-----Original Message-----
From: Jeff Law <jeffreyalaw@gmail.com>
Sent: Monday, March 4, 2024 6:47 AM
To: Robin Dapp <rdapp.gcc@gmail.com>; Li, Pan2 <pan2.li@intel.com>; gcc-patches@gcc.gnu.org
Cc: juzhe.zhong@rivai.ai; kito.cheng@gmail.com; richard.guenther@gmail.com; Wang, Yanzhang <yanzhang.wang@intel.com>; Liu, Hongtao <hongtao.liu@intel.com>
Subject: Re: [PATCH v2] DSE: Bugfix ICE after allow vector type in get_stored_val
On 2/29/24 06:28, Robin Dapp wrote:
> On 2/29/24 02:38, Li, Pan2 wrote:
>>> So it's going to check if V2SF can be tied to DI and V4QI with SI. I
>>> suspect those are going to fail for RISC-V as those aren't tieable.
>>
>> Yes, you are right. Different REG_CLASS are not allowed to be tieable in RISC-V.
>>
>> static bool
>> riscv_modes_tieable_p (machine_mode mode1, machine_mode mode2)
>> {
>> /* We don't allow different REG_CLASS modes tieable since it
>> will cause ICE in register allocation (RA).
>> E.g. V2SI and DI are not tieable. */
>> if (riscv_v_ext_mode_p (mode1) != riscv_v_ext_mode_p (mode2))
>> return false;
>> return (mode1 == mode2
>> || !(GET_MODE_CLASS (mode1) == MODE_FLOAT
>> && GET_MODE_CLASS (mode2) == MODE_FLOAT));
>> }
>
> Yes, but what we set tieable is e.g. V4QI and V2SF.
But in the case of a vector modes, we can usually reinterpret the
underlying bits in whatever mode we want and do any of the usual
operations on those bits.
In my mind that's fundamentally different than the int vs fp case. If
we have an integer value in an FP register, we can't really operate on
the value in any sensible way without first copying it over to the
integer register file and vice-versa.
Jeff
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v2] DSE: Bugfix ICE after allow vector type in get_stored_val
2024-03-05 6:22 ` Li, Pan2
2024-03-12 2:08 ` Li, Pan2
@ 2024-03-22 18:53 ` Jeff Law
2024-03-23 5:45 ` Li, Pan2
1 sibling, 1 reply; 31+ messages in thread
From: Jeff Law @ 2024-03-22 18:53 UTC (permalink / raw)
To: Li, Pan2, Robin Dapp, gcc-patches
Cc: juzhe.zhong, kito.cheng, richard.guenther, Wang, Yanzhang, Liu, Hongtao
On 3/4/24 11:22 PM, Li, Pan2 wrote:
> Thanks Jeff for comments.
>
>> But in the case of a vector modes, we can usually reinterpret the
>> underlying bits in whatever mode we want and do any of the usual
>> operations on those bits.
>
> Yes, I think that is why we can allow vector mode in get_stored_val if my understanding is correct.
> And then the different modes will return by gen_low_part. Unfortunately, there are some modes
> (less than a vector bit size like V2SF, V2QI for vlen=128) are considered as invalid by validate_subreg,
> and return NULL_RTX result in the final ICE.
That doesn't make a lot of sense to me. Even for vlen=128 I would have
expected that we can still use a subreg to access low bits. After all
we might have had a V16QI vector and done a reduction of some sort
storing the result in the first element and we have to be able to
extract that result and move it around.
I'm not real keen on a target workaround. While extremely safe, I
wouldn't be surprised if other ports could trigger the ICE and we'd end
up patching up multiple targets for what is, IMHO, a more generic issue.
As Richi noted using validate_subreg here isn't great. Does it work to
factor out this code from extract_low_bits:
> if (!int_mode_for_mode (src_mode).exists (&src_int_mode)
> || !int_mode_for_mode (mode).exists (&int_mode))
> return NULL_RTX;
>
> if (!targetm.modes_tieable_p (src_int_mode, src_mode))
> return NULL_RTX;
> if (!targetm.modes_tieable_p (int_mode, mode))
> return NULL_RTX;
And use that in the condition (and in extract_low_bits rather than
duplicating the code)?
jeff
ps. No need to apologize for the pings. This completely fell off my radar.
^ permalink raw reply [flat|nested] 31+ messages in thread
* RE: [PATCH v2] DSE: Bugfix ICE after allow vector type in get_stored_val
2024-03-22 18:53 ` Jeff Law
@ 2024-03-23 5:45 ` Li, Pan2
2024-04-06 12:02 ` Li, Pan2
2024-04-29 15:20 ` Jeff Law
0 siblings, 2 replies; 31+ messages in thread
From: Li, Pan2 @ 2024-03-23 5:45 UTC (permalink / raw)
To: Jeff Law, Robin Dapp, gcc-patches
Cc: juzhe.zhong, kito.cheng, richard.guenther, Wang, Yanzhang, Liu, Hongtao
Thanks Jeff for comments.
> As Richi noted using validate_subreg here isn't great. Does it work to
> factor out this code from extract_low_bits
>
>> if (!int_mode_for_mode (src_mode).exists (&src_int_mode)
>> || !int_mode_for_mode (mode).exists (&int_mode))
>> return NULL_RTX;
>>
>> if (!targetm.modes_tieable_p (src_int_mode, src_mode))
>> return NULL_RTX;
>> if (!targetm.modes_tieable_p (int_mode, mode))
>> return NULL_RTX;
> And use that in the condition (and in extract_low_bits rather than
> duplicating the code)?
It can solve the ICE but will forbid all vector modes goes gen_lowpart.
Actually only the vector mode size is less than reg nature size will trigger the ICE.
Thus, how about just add one more condition before goes to gen_lowpart as below?
Feel free to correct me if any misunderstandings. 😉!
diff --git a/gcc/dse.cc b/gcc/dse.cc
index edc7a1dfecf..258d2ccc299 100644
--- a/gcc/dse.cc
+++ b/gcc/dse.cc
@@ -1946,7 +1946,9 @@ get_stored_val (store_info *store_info, machine_mode read_mode,
copy_rtx (store_info->const_rhs));
else if (VECTOR_MODE_P (read_mode) && VECTOR_MODE_P (store_mode)
&& known_le (GET_MODE_BITSIZE (read_mode), GET_MODE_BITSIZE (store_mode))
- && targetm.modes_tieable_p (read_mode, store_mode))
+ && targetm.modes_tieable_p (read_mode, store_mode)
+ /* It's invalid in validate_subreg if read_mode size is < reg natural. */
+ && known_ge (GET_MODE_SIZE (read_mode), REGMODE_NATURAL_SIZE (read_mode)))
read_reg = gen_lowpart (read_mode, copy_rtx (store_info->rhs));
else
read_reg = extract_low_bits (read_mode, store_mode,
Pan
-----Original Message-----
From: Jeff Law <jeffreyalaw@gmail.com>
Sent: Saturday, March 23, 2024 2:54 AM
To: Li, Pan2 <pan2.li@intel.com>; Robin Dapp <rdapp.gcc@gmail.com>; gcc-patches@gcc.gnu.org
Cc: juzhe.zhong@rivai.ai; kito.cheng@gmail.com; richard.guenther@gmail.com; Wang, Yanzhang <yanzhang.wang@intel.com>; Liu, Hongtao <hongtao.liu@intel.com>
Subject: Re: [PATCH v2] DSE: Bugfix ICE after allow vector type in get_stored_val
On 3/4/24 11:22 PM, Li, Pan2 wrote:
> Thanks Jeff for comments.
>
>> But in the case of a vector modes, we can usually reinterpret the
>> underlying bits in whatever mode we want and do any of the usual
>> operations on those bits.
>
> Yes, I think that is why we can allow vector mode in get_stored_val if my understanding is correct.
> And then the different modes will return by gen_low_part. Unfortunately, there are some modes
> (less than a vector bit size like V2SF, V2QI for vlen=128) are considered as invalid by validate_subreg,
> and return NULL_RTX result in the final ICE.
That doesn't make a lot of sense to me. Even for vlen=128 I would have
expected that we can still use a subreg to access low bits. After all
we might have had a V16QI vector and done a reduction of some sort
storing the result in the first element and we have to be able to
extract that result and move it around.
I'm not real keen on a target workaround. While extremely safe, I
wouldn't be surprised if other ports could trigger the ICE and we'd end
up patching up multiple targets for what is, IMHO, a more generic issue.
As Richi noted using validate_subreg here isn't great. Does it work to
factor out this code from extract_low_bits:
> if (!int_mode_for_mode (src_mode).exists (&src_int_mode)
> || !int_mode_for_mode (mode).exists (&int_mode))
> return NULL_RTX;
>
> if (!targetm.modes_tieable_p (src_int_mode, src_mode))
> return NULL_RTX;
> if (!targetm.modes_tieable_p (int_mode, mode))
> return NULL_RTX;
And use that in the condition (and in extract_low_bits rather than
duplicating the code)?
jeff
ps. No need to apologize for the pings. This completely fell off my radar.
^ permalink raw reply [flat|nested] 31+ messages in thread
* RE: [PATCH v2] DSE: Bugfix ICE after allow vector type in get_stored_val
2024-03-23 5:45 ` Li, Pan2
@ 2024-04-06 12:02 ` Li, Pan2
2024-04-18 1:46 ` Li, Pan2
2024-04-29 15:20 ` Jeff Law
1 sibling, 1 reply; 31+ messages in thread
From: Li, Pan2 @ 2024-04-06 12:02 UTC (permalink / raw)
To: Li, Pan2, Jeff Law, Robin Dapp, gcc-patches
Cc: juzhe.zhong, kito.cheng, richard.guenther, Wang, Yanzhang, Liu, Hongtao
Kindly ping for this ice.
Pan
-----Original Message-----
From: Li, Pan2 <pan2.li@intel.com>
Sent: Saturday, March 23, 2024 1:45 PM
To: Jeff Law <jeffreyalaw@gmail.com>; Robin Dapp <rdapp.gcc@gmail.com>; gcc-patches@gcc.gnu.org
Cc: juzhe.zhong@rivai.ai; kito.cheng@gmail.com; richard.guenther@gmail.com; Wang, Yanzhang <yanzhang.wang@intel.com>; Liu, Hongtao <hongtao.liu@intel.com>
Subject: RE: [PATCH v2] DSE: Bugfix ICE after allow vector type in get_stored_val
Thanks Jeff for comments.
> As Richi noted using validate_subreg here isn't great. Does it work to
> factor out this code from extract_low_bits
>
>> if (!int_mode_for_mode (src_mode).exists (&src_int_mode)
>> || !int_mode_for_mode (mode).exists (&int_mode))
>> return NULL_RTX;
>>
>> if (!targetm.modes_tieable_p (src_int_mode, src_mode))
>> return NULL_RTX;
>> if (!targetm.modes_tieable_p (int_mode, mode))
>> return NULL_RTX;
> And use that in the condition (and in extract_low_bits rather than
> duplicating the code)?
It can solve the ICE but will forbid all vector modes goes gen_lowpart.
Actually only the vector mode size is less than reg nature size will trigger the ICE.
Thus, how about just add one more condition before goes to gen_lowpart as below?
Feel free to correct me if any misunderstandings. 😉!
diff --git a/gcc/dse.cc b/gcc/dse.cc
index edc7a1dfecf..258d2ccc299 100644
--- a/gcc/dse.cc
+++ b/gcc/dse.cc
@@ -1946,7 +1946,9 @@ get_stored_val (store_info *store_info, machine_mode read_mode,
copy_rtx (store_info->const_rhs));
else if (VECTOR_MODE_P (read_mode) && VECTOR_MODE_P (store_mode)
&& known_le (GET_MODE_BITSIZE (read_mode), GET_MODE_BITSIZE (store_mode))
- && targetm.modes_tieable_p (read_mode, store_mode))
+ && targetm.modes_tieable_p (read_mode, store_mode)
+ /* It's invalid in validate_subreg if read_mode size is < reg natural. */
+ && known_ge (GET_MODE_SIZE (read_mode), REGMODE_NATURAL_SIZE (read_mode)))
read_reg = gen_lowpart (read_mode, copy_rtx (store_info->rhs));
else
read_reg = extract_low_bits (read_mode, store_mode,
Pan
-----Original Message-----
From: Jeff Law <jeffreyalaw@gmail.com>
Sent: Saturday, March 23, 2024 2:54 AM
To: Li, Pan2 <pan2.li@intel.com>; Robin Dapp <rdapp.gcc@gmail.com>; gcc-patches@gcc.gnu.org
Cc: juzhe.zhong@rivai.ai; kito.cheng@gmail.com; richard.guenther@gmail.com; Wang, Yanzhang <yanzhang.wang@intel.com>; Liu, Hongtao <hongtao.liu@intel.com>
Subject: Re: [PATCH v2] DSE: Bugfix ICE after allow vector type in get_stored_val
On 3/4/24 11:22 PM, Li, Pan2 wrote:
> Thanks Jeff for comments.
>
>> But in the case of a vector modes, we can usually reinterpret the
>> underlying bits in whatever mode we want and do any of the usual
>> operations on those bits.
>
> Yes, I think that is why we can allow vector mode in get_stored_val if my understanding is correct.
> And then the different modes will return by gen_low_part. Unfortunately, there are some modes
> (less than a vector bit size like V2SF, V2QI for vlen=128) are considered as invalid by validate_subreg,
> and return NULL_RTX result in the final ICE.
That doesn't make a lot of sense to me. Even for vlen=128 I would have
expected that we can still use a subreg to access low bits. After all
we might have had a V16QI vector and done a reduction of some sort
storing the result in the first element and we have to be able to
extract that result and move it around.
I'm not real keen on a target workaround. While extremely safe, I
wouldn't be surprised if other ports could trigger the ICE and we'd end
up patching up multiple targets for what is, IMHO, a more generic issue.
As Richi noted using validate_subreg here isn't great. Does it work to
factor out this code from extract_low_bits:
> if (!int_mode_for_mode (src_mode).exists (&src_int_mode)
> || !int_mode_for_mode (mode).exists (&int_mode))
> return NULL_RTX;
>
> if (!targetm.modes_tieable_p (src_int_mode, src_mode))
> return NULL_RTX;
> if (!targetm.modes_tieable_p (int_mode, mode))
> return NULL_RTX;
And use that in the condition (and in extract_low_bits rather than
duplicating the code)?
jeff
ps. No need to apologize for the pings. This completely fell off my radar.
^ permalink raw reply [flat|nested] 31+ messages in thread
* RE: [PATCH v2] DSE: Bugfix ICE after allow vector type in get_stored_val
2024-04-06 12:02 ` Li, Pan2
@ 2024-04-18 1:46 ` Li, Pan2
2024-04-28 12:10 ` Li, Pan2
0 siblings, 1 reply; 31+ messages in thread
From: Li, Pan2 @ 2024-04-18 1:46 UTC (permalink / raw)
To: Jeff Law, Robin Dapp, gcc-patches
Cc: juzhe.zhong, kito.cheng, richard.guenther, Liu, Hongtao
Kindly ping^ for this ice fix.
Pan
-----Original Message-----
From: Li, Pan2
Sent: Saturday, April 6, 2024 8:02 PM
To: Li, Pan2 <pan2.li@intel.com>; Jeff Law <jeffreyalaw@gmail.com>; Robin Dapp <rdapp.gcc@gmail.com>; gcc-patches@gcc.gnu.org
Cc: juzhe.zhong@rivai.ai; kito.cheng@gmail.com; richard.guenther@gmail.com; Wang, Yanzhang <yanzhang.wang@intel.com>; Liu, Hongtao <Hongtao.Liu@intel.com>
Subject: RE: [PATCH v2] DSE: Bugfix ICE after allow vector type in get_stored_val
Kindly ping for this ice.
Pan
-----Original Message-----
From: Li, Pan2 <pan2.li@intel.com>
Sent: Saturday, March 23, 2024 1:45 PM
To: Jeff Law <jeffreyalaw@gmail.com>; Robin Dapp <rdapp.gcc@gmail.com>; gcc-patches@gcc.gnu.org
Cc: juzhe.zhong@rivai.ai; kito.cheng@gmail.com; richard.guenther@gmail.com; Wang, Yanzhang <yanzhang.wang@intel.com>; Liu, Hongtao <hongtao.liu@intel.com>
Subject: RE: [PATCH v2] DSE: Bugfix ICE after allow vector type in get_stored_val
Thanks Jeff for comments.
> As Richi noted using validate_subreg here isn't great. Does it work to
> factor out this code from extract_low_bits
>
>> if (!int_mode_for_mode (src_mode).exists (&src_int_mode)
>> || !int_mode_for_mode (mode).exists (&int_mode))
>> return NULL_RTX;
>>
>> if (!targetm.modes_tieable_p (src_int_mode, src_mode))
>> return NULL_RTX;
>> if (!targetm.modes_tieable_p (int_mode, mode))
>> return NULL_RTX;
> And use that in the condition (and in extract_low_bits rather than
> duplicating the code)?
It can solve the ICE but will forbid all vector modes goes gen_lowpart.
Actually only the vector mode size is less than reg nature size will trigger the ICE.
Thus, how about just add one more condition before goes to gen_lowpart as below?
Feel free to correct me if any misunderstandings. 😉!
diff --git a/gcc/dse.cc b/gcc/dse.cc
index edc7a1dfecf..258d2ccc299 100644
--- a/gcc/dse.cc
+++ b/gcc/dse.cc
@@ -1946,7 +1946,9 @@ get_stored_val (store_info *store_info, machine_mode read_mode,
copy_rtx (store_info->const_rhs));
else if (VECTOR_MODE_P (read_mode) && VECTOR_MODE_P (store_mode)
&& known_le (GET_MODE_BITSIZE (read_mode), GET_MODE_BITSIZE (store_mode))
- && targetm.modes_tieable_p (read_mode, store_mode))
+ && targetm.modes_tieable_p (read_mode, store_mode)
+ /* It's invalid in validate_subreg if read_mode size is < reg natural. */
+ && known_ge (GET_MODE_SIZE (read_mode), REGMODE_NATURAL_SIZE (read_mode)))
read_reg = gen_lowpart (read_mode, copy_rtx (store_info->rhs));
else
read_reg = extract_low_bits (read_mode, store_mode,
Pan
-----Original Message-----
From: Jeff Law <jeffreyalaw@gmail.com>
Sent: Saturday, March 23, 2024 2:54 AM
To: Li, Pan2 <pan2.li@intel.com>; Robin Dapp <rdapp.gcc@gmail.com>; gcc-patches@gcc.gnu.org
Cc: juzhe.zhong@rivai.ai; kito.cheng@gmail.com; richard.guenther@gmail.com; Wang, Yanzhang <yanzhang.wang@intel.com>; Liu, Hongtao <hongtao.liu@intel.com>
Subject: Re: [PATCH v2] DSE: Bugfix ICE after allow vector type in get_stored_val
On 3/4/24 11:22 PM, Li, Pan2 wrote:
> Thanks Jeff for comments.
>
>> But in the case of a vector modes, we can usually reinterpret the
>> underlying bits in whatever mode we want and do any of the usual
>> operations on those bits.
>
> Yes, I think that is why we can allow vector mode in get_stored_val if my understanding is correct.
> And then the different modes will return by gen_low_part. Unfortunately, there are some modes
> (less than a vector bit size like V2SF, V2QI for vlen=128) are considered as invalid by validate_subreg,
> and return NULL_RTX result in the final ICE.
That doesn't make a lot of sense to me. Even for vlen=128 I would have
expected that we can still use a subreg to access low bits. After all
we might have had a V16QI vector and done a reduction of some sort
storing the result in the first element and we have to be able to
extract that result and move it around.
I'm not real keen on a target workaround. While extremely safe, I
wouldn't be surprised if other ports could trigger the ICE and we'd end
up patching up multiple targets for what is, IMHO, a more generic issue.
As Richi noted using validate_subreg here isn't great. Does it work to
factor out this code from extract_low_bits:
> if (!int_mode_for_mode (src_mode).exists (&src_int_mode)
> || !int_mode_for_mode (mode).exists (&int_mode))
> return NULL_RTX;
>
> if (!targetm.modes_tieable_p (src_int_mode, src_mode))
> return NULL_RTX;
> if (!targetm.modes_tieable_p (int_mode, mode))
> return NULL_RTX;
And use that in the condition (and in extract_low_bits rather than
duplicating the code)?
jeff
ps. No need to apologize for the pings. This completely fell off my radar.
^ permalink raw reply [flat|nested] 31+ messages in thread
* RE: [PATCH v2] DSE: Bugfix ICE after allow vector type in get_stored_val
2024-04-18 1:46 ` Li, Pan2
@ 2024-04-28 12:10 ` Li, Pan2
0 siblings, 0 replies; 31+ messages in thread
From: Li, Pan2 @ 2024-04-28 12:10 UTC (permalink / raw)
To: Li, Pan2, Jeff Law, Robin Dapp, gcc-patches
Cc: juzhe.zhong, kito.cheng, richard.guenther, Liu, Hongtao
Kindly ping^^ for this ice fix.
Pan
-----Original Message-----
From: Li, Pan2 <pan2.li@intel.com>
Sent: Thursday, April 18, 2024 9:46 AM
To: Jeff Law <jeffreyalaw@gmail.com>; Robin Dapp <rdapp.gcc@gmail.com>; gcc-patches@gcc.gnu.org
Cc: juzhe.zhong@rivai.ai; kito.cheng@gmail.com; richard.guenther@gmail.com; Liu, Hongtao <hongtao.liu@intel.com>
Subject: RE: [PATCH v2] DSE: Bugfix ICE after allow vector type in get_stored_val
Kindly ping^ for this ice fix.
Pan
-----Original Message-----
From: Li, Pan2
Sent: Saturday, April 6, 2024 8:02 PM
To: Li, Pan2 <pan2.li@intel.com>; Jeff Law <jeffreyalaw@gmail.com>; Robin Dapp <rdapp.gcc@gmail.com>; gcc-patches@gcc.gnu.org
Cc: juzhe.zhong@rivai.ai; kito.cheng@gmail.com; richard.guenther@gmail.com; Wang, Yanzhang <yanzhang.wang@intel.com>; Liu, Hongtao <Hongtao.Liu@intel.com>
Subject: RE: [PATCH v2] DSE: Bugfix ICE after allow vector type in get_stored_val
Kindly ping for this ice.
Pan
-----Original Message-----
From: Li, Pan2 <pan2.li@intel.com>
Sent: Saturday, March 23, 2024 1:45 PM
To: Jeff Law <jeffreyalaw@gmail.com>; Robin Dapp <rdapp.gcc@gmail.com>; gcc-patches@gcc.gnu.org
Cc: juzhe.zhong@rivai.ai; kito.cheng@gmail.com; richard.guenther@gmail.com; Wang, Yanzhang <yanzhang.wang@intel.com>; Liu, Hongtao <hongtao.liu@intel.com>
Subject: RE: [PATCH v2] DSE: Bugfix ICE after allow vector type in get_stored_val
Thanks Jeff for comments.
> As Richi noted using validate_subreg here isn't great. Does it work to
> factor out this code from extract_low_bits
>
>> if (!int_mode_for_mode (src_mode).exists (&src_int_mode)
>> || !int_mode_for_mode (mode).exists (&int_mode))
>> return NULL_RTX;
>>
>> if (!targetm.modes_tieable_p (src_int_mode, src_mode))
>> return NULL_RTX;
>> if (!targetm.modes_tieable_p (int_mode, mode))
>> return NULL_RTX;
> And use that in the condition (and in extract_low_bits rather than
> duplicating the code)?
It can solve the ICE but will forbid all vector modes goes gen_lowpart.
Actually only the vector mode size is less than reg nature size will trigger the ICE.
Thus, how about just add one more condition before goes to gen_lowpart as below?
Feel free to correct me if any misunderstandings. 😉!
diff --git a/gcc/dse.cc b/gcc/dse.cc
index edc7a1dfecf..258d2ccc299 100644
--- a/gcc/dse.cc
+++ b/gcc/dse.cc
@@ -1946,7 +1946,9 @@ get_stored_val (store_info *store_info, machine_mode read_mode,
copy_rtx (store_info->const_rhs));
else if (VECTOR_MODE_P (read_mode) && VECTOR_MODE_P (store_mode)
&& known_le (GET_MODE_BITSIZE (read_mode), GET_MODE_BITSIZE (store_mode))
- && targetm.modes_tieable_p (read_mode, store_mode))
+ && targetm.modes_tieable_p (read_mode, store_mode)
+ /* It's invalid in validate_subreg if read_mode size is < reg natural. */
+ && known_ge (GET_MODE_SIZE (read_mode), REGMODE_NATURAL_SIZE (read_mode)))
read_reg = gen_lowpart (read_mode, copy_rtx (store_info->rhs));
else
read_reg = extract_low_bits (read_mode, store_mode,
Pan
-----Original Message-----
From: Jeff Law <jeffreyalaw@gmail.com>
Sent: Saturday, March 23, 2024 2:54 AM
To: Li, Pan2 <pan2.li@intel.com>; Robin Dapp <rdapp.gcc@gmail.com>; gcc-patches@gcc.gnu.org
Cc: juzhe.zhong@rivai.ai; kito.cheng@gmail.com; richard.guenther@gmail.com; Wang, Yanzhang <yanzhang.wang@intel.com>; Liu, Hongtao <hongtao.liu@intel.com>
Subject: Re: [PATCH v2] DSE: Bugfix ICE after allow vector type in get_stored_val
On 3/4/24 11:22 PM, Li, Pan2 wrote:
> Thanks Jeff for comments.
>
>> But in the case of a vector modes, we can usually reinterpret the
>> underlying bits in whatever mode we want and do any of the usual
>> operations on those bits.
>
> Yes, I think that is why we can allow vector mode in get_stored_val if my understanding is correct.
> And then the different modes will return by gen_low_part. Unfortunately, there are some modes
> (less than a vector bit size like V2SF, V2QI for vlen=128) are considered as invalid by validate_subreg,
> and return NULL_RTX result in the final ICE.
That doesn't make a lot of sense to me. Even for vlen=128 I would have
expected that we can still use a subreg to access low bits. After all
we might have had a V16QI vector and done a reduction of some sort
storing the result in the first element and we have to be able to
extract that result and move it around.
I'm not real keen on a target workaround. While extremely safe, I
wouldn't be surprised if other ports could trigger the ICE and we'd end
up patching up multiple targets for what is, IMHO, a more generic issue.
As Richi noted using validate_subreg here isn't great. Does it work to
factor out this code from extract_low_bits:
> if (!int_mode_for_mode (src_mode).exists (&src_int_mode)
> || !int_mode_for_mode (mode).exists (&int_mode))
> return NULL_RTX;
>
> if (!targetm.modes_tieable_p (src_int_mode, src_mode))
> return NULL_RTX;
> if (!targetm.modes_tieable_p (int_mode, mode))
> return NULL_RTX;
And use that in the condition (and in extract_low_bits rather than
duplicating the code)?
jeff
ps. No need to apologize for the pings. This completely fell off my radar.
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v2] DSE: Bugfix ICE after allow vector type in get_stored_val
2024-03-23 5:45 ` Li, Pan2
2024-04-06 12:02 ` Li, Pan2
@ 2024-04-29 15:20 ` Jeff Law
2024-04-30 1:02 ` Li, Pan2
1 sibling, 1 reply; 31+ messages in thread
From: Jeff Law @ 2024-04-29 15:20 UTC (permalink / raw)
To: Li, Pan2, Robin Dapp, gcc-patches
Cc: juzhe.zhong, kito.cheng, richard.guenther, Wang, Yanzhang, Liu, Hongtao
On 3/22/24 11:45 PM, Li, Pan2 wrote:
> Thanks Jeff for comments.
>
>> As Richi noted using validate_subreg here isn't great. Does it work to
>> factor out this code from extract_low_bits
>>
>>> if (!int_mode_for_mode (src_mode).exists (&src_int_mode)
>>> || !int_mode_for_mode (mode).exists (&int_mode))
>>> return NULL_RTX;
>>>
>>> if (!targetm.modes_tieable_p (src_int_mode, src_mode))
>>> return NULL_RTX;
>>> if (!targetm.modes_tieable_p (int_mode, mode))
>>> return NULL_RTX;
>
>> And use that in the condition (and in extract_low_bits rather than
>> duplicating the code)?
>
> It can solve the ICE but will forbid all vector modes goes gen_lowpart.
> Actually only the vector mode size is less than reg nature size will trigger the ICE.
> Thus, how about just add one more condition before goes to gen_lowpart as below?
>
> Feel free to correct me if any misunderstandings. 😉!
>
> diff --git a/gcc/dse.cc b/gcc/dse.cc
> index edc7a1dfecf..258d2ccc299 100644
> --- a/gcc/dse.cc
> +++ b/gcc/dse.cc
> @@ -1946,7 +1946,9 @@ get_stored_val (store_info *store_info, machine_mode read_mode,
> copy_rtx (store_info->const_rhs));
> else if (VECTOR_MODE_P (read_mode) && VECTOR_MODE_P (store_mode)
> && known_le (GET_MODE_BITSIZE (read_mode), GET_MODE_BITSIZE (store_mode))
> - && targetm.modes_tieable_p (read_mode, store_mode))
> + && targetm.modes_tieable_p (read_mode, store_mode)
> + /* It's invalid in validate_subreg if read_mode size is < reg natural. */
> + && known_ge (GET_MODE_SIZE (read_mode), REGMODE_NATURAL_SIZE (read_mode)))
> read_reg = gen_lowpart (read_mode, copy_rtx (store_info->rhs));
> else
> read_reg = extract_low_bits (read_mode, store_mode,
So how about this. I'll ack this for the trunk, but not for gcc-14 (at
this time). We can revisit for gcc-14 after it's been on the trunk a
bit. Realistically that likely means gcc-14.2 at the end of the summer
rather than gcc-14.1 which is due in roughly a week.
Jeff
^ permalink raw reply [flat|nested] 31+ messages in thread
* RE: [PATCH v2] DSE: Bugfix ICE after allow vector type in get_stored_val
2024-04-29 15:20 ` Jeff Law
@ 2024-04-30 1:02 ` Li, Pan2
0 siblings, 0 replies; 31+ messages in thread
From: Li, Pan2 @ 2024-04-30 1:02 UTC (permalink / raw)
To: Jeff Law, Robin Dapp, gcc-patches
Cc: juzhe.zhong, kito.cheng, richard.guenther, Liu, Hongtao
> So how about this. I'll ack this for the trunk, but not for gcc-14 (at
> this time). We can revisit for gcc-14 after it's been on the trunk a
> bit. Realistically that likely means gcc-14.2 at the end of the summer
> rather than gcc-14.1 which is due in roughly a week.
Thanks Jeff, I will prepare a v3 for this with full and well tested results.
Pan
-----Original Message-----
From: Jeff Law <jeffreyalaw@gmail.com>
Sent: Monday, April 29, 2024 11:20 PM
To: Li, Pan2 <pan2.li@intel.com>; Robin Dapp <rdapp.gcc@gmail.com>; gcc-patches@gcc.gnu.org
Cc: juzhe.zhong@rivai.ai; kito.cheng@gmail.com; richard.guenther@gmail.com; Wang, Yanzhang <yanzhang.wang@intel.com>; Liu, Hongtao <hongtao.liu@intel.com>
Subject: Re: [PATCH v2] DSE: Bugfix ICE after allow vector type in get_stored_val
On 3/22/24 11:45 PM, Li, Pan2 wrote:
> Thanks Jeff for comments.
>
>> As Richi noted using validate_subreg here isn't great. Does it work to
>> factor out this code from extract_low_bits
>>
>>> if (!int_mode_for_mode (src_mode).exists (&src_int_mode)
>>> || !int_mode_for_mode (mode).exists (&int_mode))
>>> return NULL_RTX;
>>>
>>> if (!targetm.modes_tieable_p (src_int_mode, src_mode))
>>> return NULL_RTX;
>>> if (!targetm.modes_tieable_p (int_mode, mode))
>>> return NULL_RTX;
>
>> And use that in the condition (and in extract_low_bits rather than
>> duplicating the code)?
>
> It can solve the ICE but will forbid all vector modes goes gen_lowpart.
> Actually only the vector mode size is less than reg nature size will trigger the ICE.
> Thus, how about just add one more condition before goes to gen_lowpart as below?
>
> Feel free to correct me if any misunderstandings. 😉!
>
> diff --git a/gcc/dse.cc b/gcc/dse.cc
> index edc7a1dfecf..258d2ccc299 100644
> --- a/gcc/dse.cc
> +++ b/gcc/dse.cc
> @@ -1946,7 +1946,9 @@ get_stored_val (store_info *store_info, machine_mode read_mode,
> copy_rtx (store_info->const_rhs));
> else if (VECTOR_MODE_P (read_mode) && VECTOR_MODE_P (store_mode)
> && known_le (GET_MODE_BITSIZE (read_mode), GET_MODE_BITSIZE (store_mode))
> - && targetm.modes_tieable_p (read_mode, store_mode))
> + && targetm.modes_tieable_p (read_mode, store_mode)
> + /* It's invalid in validate_subreg if read_mode size is < reg natural. */
> + && known_ge (GET_MODE_SIZE (read_mode), REGMODE_NATURAL_SIZE (read_mode)))
> read_reg = gen_lowpart (read_mode, copy_rtx (store_info->rhs));
> else
> read_reg = extract_low_bits (read_mode, store_mode,
So how about this. I'll ack this for the trunk, but not for gcc-14 (at
this time). We can revisit for gcc-14 after it's been on the trunk a
bit. Realistically that likely means gcc-14.2 at the end of the summer
rather than gcc-14.1 which is due in roughly a week.
Jeff
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH v3] DSE: Fix ICE after allow vector type in get_stored_val
2024-02-26 3:25 [PATCH v1] RTL: Bugfix ICE after allow vector type in DSE pan2.li
` (2 preceding siblings ...)
2024-02-26 14:22 ` [PATCH v2] DSE: Bugfix ICE after allow vector type in get_stored_val pan2.li
@ 2024-04-30 7:17 ` pan2.li
2024-04-30 11:35 ` Li, Pan2
2024-05-03 1:51 ` [PATCH v4] " pan2.li
4 siblings, 1 reply; 31+ messages in thread
From: pan2.li @ 2024-04-30 7:17 UTC (permalink / raw)
To: gcc-patches
Cc: jeffreyalaw, juzhe.zhong, kito.cheng, Hongtao.Liu,
richard.guenther, Pan Li
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, the valididate_subreg
treats the vector type's size is less than vector register as
invalid. Then we will have ICE here.
This patch would like to fix it by filter-out the invalid type size,
and make sure the subreg is valid for both the read_mode and store_mode
before perform the real gen_lowpart.
The below test suites are passed for this patch:
* The x86 bootstrap test.
* The x86 regression test.
* The riscv rv64gcv regression test.
* The riscv rv64gc regression test.
* The aarch64 regression test.
gcc/ChangeLog:
* dse.cc (get_stored_val): Make sure read_mode size is greater
than or equal to the vector register size before gen_lowpart.
gcc/testsuite/ChangeLog:
* gcc.target/riscv/rvv/base/pr111720-10.c: Adjust asm checker.
* gcc.target/riscv/rvv/base/bug-6.c: New test.
Signed-off-by: Pan Li <pan2.li@intel.com>
---
gcc/dse.cc | 4 +++-
.../gcc.target/riscv/rvv/base/bug-6.c | 22 +++++++++++++++++++
.../gcc.target/riscv/rvv/base/pr111720-10.c | 2 +-
3 files changed, 26 insertions(+), 2 deletions(-)
create mode 100644 gcc/testsuite/gcc.target/riscv/rvv/base/bug-6.c
diff --git a/gcc/dse.cc b/gcc/dse.cc
index edc7a1dfecf..258d2ccc299 100644
--- a/gcc/dse.cc
+++ b/gcc/dse.cc
@@ -1946,7 +1946,9 @@ get_stored_val (store_info *store_info, machine_mode read_mode,
copy_rtx (store_info->const_rhs));
else if (VECTOR_MODE_P (read_mode) && VECTOR_MODE_P (store_mode)
&& known_le (GET_MODE_BITSIZE (read_mode), GET_MODE_BITSIZE (store_mode))
- && targetm.modes_tieable_p (read_mode, store_mode))
+ && targetm.modes_tieable_p (read_mode, store_mode)
+ /* It's invalid in validate_subreg if read_mode size is < reg natural. */
+ && known_ge (GET_MODE_SIZE (read_mode), REGMODE_NATURAL_SIZE (read_mode)))
read_reg = gen_lowpart (read_mode, copy_rtx (store_info->rhs));
else
read_reg = extract_low_bits (read_mode, store_mode,
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;
+}
diff --git a/gcc/testsuite/gcc.target/riscv/rvv/base/pr111720-10.c b/gcc/testsuite/gcc.target/riscv/rvv/base/pr111720-10.c
index 215eb99ce0f..ee6b2ccf7ad 100644
--- a/gcc/testsuite/gcc.target/riscv/rvv/base/pr111720-10.c
+++ b/gcc/testsuite/gcc.target/riscv/rvv/base/pr111720-10.c
@@ -15,4 +15,4 @@ vbool4_t test () {
}
/* { dg-final { scan-assembler-not {vle[0-9]+\.v\s+v[0-9]+,\s*[0-9]+\(sp\)} } } */
-/* { dg-final { scan-assembler-not {vs[0-9]+r\.v\s+v[0-9]+,\s*[0-9]+\(sp\)} } } */
+/* { dg-final { scan-assembler-times {vs[0-9]+r\.v\s+v[0-9]+,\s*[0-9]+\(sp\)} 1 } } */
--
2.34.1
^ permalink raw reply [flat|nested] 31+ messages in thread
* RE: [PATCH v3] DSE: Fix ICE after allow vector type in get_stored_val
2024-04-30 7:17 ` [PATCH v3] DSE: Fix " pan2.li
@ 2024-04-30 11:35 ` Li, Pan2
2024-05-03 1:57 ` Li, Pan2
0 siblings, 1 reply; 31+ messages in thread
From: Li, Pan2 @ 2024-04-30 11:35 UTC (permalink / raw)
To: gcc-patches
Cc: jeffreyalaw, juzhe.zhong, kito.cheng, Liu, Hongtao, richard.guenther
Linaro reports one build failure for arm but works well in aarch64.
/home/tcwg-build/workspace/tcwg_gnu_2/abe/snapshots/gcc.git~master/gcc/dse.cc:1951:45: error: 'REGMODE_NATURAL_SIZE' was not declared in this scope
Looks only part of backend implemented REGMODE_NATURAL_SIZE, then reference this macro here may not be a good idea.
gcc/config/i386/i386.cc:20835:/* Implement REGMODE_NATURAL_SIZE(MODE). */
gcc/config/i386/i386.h:1050:#define REGMODE_NATURAL_SIZE(MODE) ix86_regmode_natural_size (MODE)
gcc/config/aarch64/aarch64.cc:2479:/* Implement REGMODE_NATURAL_SIZE. */
gcc/config/aarch64/aarch64.h:1505:#define REGMODE_NATURAL_SIZE(MODE) aarch64_regmode_natural_size (MODE)
gcc/config/riscv/riscv.h:1212:#define REGMODE_NATURAL_SIZE(MODE) riscv_regmode_natural_size (MODE)
gcc/config/riscv/riscv.cc:10241:/* Implement REGMODE_NATURAL_SIZE. */
gcc/config/sparc/sparc.h:706:#define REGMODE_NATURAL_SIZE(MODE) sparc_regmode_natural_size (MODE)
Pan
-----Original Message-----
From: Li, Pan2 <pan2.li@intel.com>
Sent: Tuesday, April 30, 2024 3:17 PM
To: gcc-patches@gcc.gnu.org
Cc: jeffreyalaw@gmail.com; juzhe.zhong@rivai.ai; kito.cheng@gmail.com; Liu, Hongtao <hongtao.liu@intel.com>; richard.guenther@gmail.com; Li, Pan2 <pan2.li@intel.com>
Subject: [PATCH v3] DSE: Fix ICE after allow vector type in get_stored_val
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, the valididate_subreg
treats the vector type's size is less than vector register as
invalid. Then we will have ICE here.
This patch would like to fix it by filter-out the invalid type size,
and make sure the subreg is valid for both the read_mode and store_mode
before perform the real gen_lowpart.
The below test suites are passed for this patch:
* The x86 bootstrap test.
* The x86 regression test.
* The riscv rv64gcv regression test.
* The riscv rv64gc regression test.
* The aarch64 regression test.
gcc/ChangeLog:
* dse.cc (get_stored_val): Make sure read_mode size is greater
than or equal to the vector register size before gen_lowpart.
gcc/testsuite/ChangeLog:
* gcc.target/riscv/rvv/base/pr111720-10.c: Adjust asm checker.
* gcc.target/riscv/rvv/base/bug-6.c: New test.
Signed-off-by: Pan Li <pan2.li@intel.com>
---
gcc/dse.cc | 4 +++-
.../gcc.target/riscv/rvv/base/bug-6.c | 22 +++++++++++++++++++
.../gcc.target/riscv/rvv/base/pr111720-10.c | 2 +-
3 files changed, 26 insertions(+), 2 deletions(-)
create mode 100644 gcc/testsuite/gcc.target/riscv/rvv/base/bug-6.c
diff --git a/gcc/dse.cc b/gcc/dse.cc
index edc7a1dfecf..258d2ccc299 100644
--- a/gcc/dse.cc
+++ b/gcc/dse.cc
@@ -1946,7 +1946,9 @@ get_stored_val (store_info *store_info, machine_mode read_mode,
copy_rtx (store_info->const_rhs));
else if (VECTOR_MODE_P (read_mode) && VECTOR_MODE_P (store_mode)
&& known_le (GET_MODE_BITSIZE (read_mode), GET_MODE_BITSIZE (store_mode))
- && targetm.modes_tieable_p (read_mode, store_mode))
+ && targetm.modes_tieable_p (read_mode, store_mode)
+ /* It's invalid in validate_subreg if read_mode size is < reg natural. */
+ && known_ge (GET_MODE_SIZE (read_mode), REGMODE_NATURAL_SIZE (read_mode)))
read_reg = gen_lowpart (read_mode, copy_rtx (store_info->rhs));
else
read_reg = extract_low_bits (read_mode, store_mode,
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;
+}
diff --git a/gcc/testsuite/gcc.target/riscv/rvv/base/pr111720-10.c b/gcc/testsuite/gcc.target/riscv/rvv/base/pr111720-10.c
index 215eb99ce0f..ee6b2ccf7ad 100644
--- a/gcc/testsuite/gcc.target/riscv/rvv/base/pr111720-10.c
+++ b/gcc/testsuite/gcc.target/riscv/rvv/base/pr111720-10.c
@@ -15,4 +15,4 @@ vbool4_t test () {
}
/* { dg-final { scan-assembler-not {vle[0-9]+\.v\s+v[0-9]+,\s*[0-9]+\(sp\)} } } */
-/* { dg-final { scan-assembler-not {vs[0-9]+r\.v\s+v[0-9]+,\s*[0-9]+\(sp\)} } } */
+/* { dg-final { scan-assembler-times {vs[0-9]+r\.v\s+v[0-9]+,\s*[0-9]+\(sp\)} 1 } } */
--
2.34.1
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH v4] DSE: Fix ICE after allow vector type in get_stored_val
2024-02-26 3:25 [PATCH v1] RTL: Bugfix ICE after allow vector type in DSE pan2.li
` (3 preceding siblings ...)
2024-04-30 7:17 ` [PATCH v3] DSE: Fix " pan2.li
@ 2024-05-03 1:51 ` pan2.li
2024-05-16 4:06 ` Li, Pan2
4 siblings, 1 reply; 31+ messages in thread
From: pan2.li @ 2024-05-03 1:51 UTC (permalink / raw)
To: gcc-patches
Cc: jeffreyalaw, juzhe.zhong, kito.cheng, Hongtao.Liu,
richard.guenther, Pan Li
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, the valididate_subreg
treats the vector type's size is less than vector register as
invalid. Then we will have ICE here.
This patch would like to fix it by filter-out the invalid type size,
and make sure the subreg is valid for both the read_mode and store_mode
before perform the real gen_lowpart.
The below test suites are passed for this patch:
* The x86 bootstrap test.
* The x86 regression test.
* The riscv rv64gcv regression test.
* The riscv rv64gc regression test.
* The aarch64 regression test.
gcc/ChangeLog:
* dse.cc (get_stored_val): Make sure read_mode/write_mode
is valid subreg before gen_lowpart.
gcc/testsuite/ChangeLog:
* gcc.target/riscv/rvv/base/bug-6.c: New test.
Signed-off-by: Pan Li <pan2.li@intel.com>
---
gcc/dse.cc | 4 +++-
.../gcc.target/riscv/rvv/base/bug-6.c | 22 +++++++++++++++++++
2 files changed, 25 insertions(+), 1 deletion(-)
create mode 100644 gcc/testsuite/gcc.target/riscv/rvv/base/bug-6.c
diff --git a/gcc/dse.cc b/gcc/dse.cc
index edc7a1dfecf..1596da91da0 100644
--- a/gcc/dse.cc
+++ b/gcc/dse.cc
@@ -1946,7 +1946,9 @@ get_stored_val (store_info *store_info, machine_mode read_mode,
copy_rtx (store_info->const_rhs));
else if (VECTOR_MODE_P (read_mode) && VECTOR_MODE_P (store_mode)
&& known_le (GET_MODE_BITSIZE (read_mode), GET_MODE_BITSIZE (store_mode))
- && targetm.modes_tieable_p (read_mode, store_mode))
+ && targetm.modes_tieable_p (read_mode, store_mode)
+ && validate_subreg (read_mode, store_mode, copy_rtx (store_info->rhs),
+ subreg_lowpart_offset (read_mode, store_mode)))
read_reg = gen_lowpart (read_mode, copy_rtx (store_info->rhs));
else
read_reg = extract_low_bits (read_mode, store_mode,
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
^ permalink raw reply [flat|nested] 31+ messages in thread
* RE: [PATCH v3] DSE: Fix ICE after allow vector type in get_stored_val
2024-04-30 11:35 ` Li, Pan2
@ 2024-05-03 1:57 ` Li, Pan2
0 siblings, 0 replies; 31+ messages in thread
From: Li, Pan2 @ 2024-05-03 1:57 UTC (permalink / raw)
To: gcc-patches
Cc: jeffreyalaw, juzhe.zhong, kito.cheng, Liu, Hongtao, richard.guenther
Try to invoke validate_subreg directly in v4 as below.
https://gcc.gnu.org/pipermail/gcc-patches/2024-May/650596.html
Pan
-----Original Message-----
From: Li, Pan2
Sent: Tuesday, April 30, 2024 7:36 PM
To: gcc-patches@gcc.gnu.org
Cc: jeffreyalaw@gmail.com; juzhe.zhong@rivai.ai; kito.cheng@gmail.com; Liu, Hongtao <Hongtao.Liu@intel.com>; richard.guenther@gmail.com
Subject: RE: [PATCH v3] DSE: Fix ICE after allow vector type in get_stored_val
Linaro reports one build failure for arm but works well in aarch64.
/home/tcwg-build/workspace/tcwg_gnu_2/abe/snapshots/gcc.git~master/gcc/dse.cc:1951:45: error: 'REGMODE_NATURAL_SIZE' was not declared in this scope
Looks only part of backend implemented REGMODE_NATURAL_SIZE, then reference this macro here may not be a good idea.
gcc/config/i386/i386.cc:20835:/* Implement REGMODE_NATURAL_SIZE(MODE). */
gcc/config/i386/i386.h:1050:#define REGMODE_NATURAL_SIZE(MODE) ix86_regmode_natural_size (MODE)
gcc/config/aarch64/aarch64.cc:2479:/* Implement REGMODE_NATURAL_SIZE. */
gcc/config/aarch64/aarch64.h:1505:#define REGMODE_NATURAL_SIZE(MODE) aarch64_regmode_natural_size (MODE)
gcc/config/riscv/riscv.h:1212:#define REGMODE_NATURAL_SIZE(MODE) riscv_regmode_natural_size (MODE)
gcc/config/riscv/riscv.cc:10241:/* Implement REGMODE_NATURAL_SIZE. */
gcc/config/sparc/sparc.h:706:#define REGMODE_NATURAL_SIZE(MODE) sparc_regmode_natural_size (MODE)
Pan
-----Original Message-----
From: Li, Pan2 <pan2.li@intel.com>
Sent: Tuesday, April 30, 2024 3:17 PM
To: gcc-patches@gcc.gnu.org
Cc: jeffreyalaw@gmail.com; juzhe.zhong@rivai.ai; kito.cheng@gmail.com; Liu, Hongtao <hongtao.liu@intel.com>; richard.guenther@gmail.com; Li, Pan2 <pan2.li@intel.com>
Subject: [PATCH v3] DSE: Fix ICE after allow vector type in get_stored_val
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, the valididate_subreg
treats the vector type's size is less than vector register as
invalid. Then we will have ICE here.
This patch would like to fix it by filter-out the invalid type size,
and make sure the subreg is valid for both the read_mode and store_mode
before perform the real gen_lowpart.
The below test suites are passed for this patch:
* The x86 bootstrap test.
* The x86 regression test.
* The riscv rv64gcv regression test.
* The riscv rv64gc regression test.
* The aarch64 regression test.
gcc/ChangeLog:
* dse.cc (get_stored_val): Make sure read_mode size is greater
than or equal to the vector register size before gen_lowpart.
gcc/testsuite/ChangeLog:
* gcc.target/riscv/rvv/base/pr111720-10.c: Adjust asm checker.
* gcc.target/riscv/rvv/base/bug-6.c: New test.
Signed-off-by: Pan Li <pan2.li@intel.com>
---
gcc/dse.cc | 4 +++-
.../gcc.target/riscv/rvv/base/bug-6.c | 22 +++++++++++++++++++
.../gcc.target/riscv/rvv/base/pr111720-10.c | 2 +-
3 files changed, 26 insertions(+), 2 deletions(-)
create mode 100644 gcc/testsuite/gcc.target/riscv/rvv/base/bug-6.c
diff --git a/gcc/dse.cc b/gcc/dse.cc
index edc7a1dfecf..258d2ccc299 100644
--- a/gcc/dse.cc
+++ b/gcc/dse.cc
@@ -1946,7 +1946,9 @@ get_stored_val (store_info *store_info, machine_mode read_mode,
copy_rtx (store_info->const_rhs));
else if (VECTOR_MODE_P (read_mode) && VECTOR_MODE_P (store_mode)
&& known_le (GET_MODE_BITSIZE (read_mode), GET_MODE_BITSIZE (store_mode))
- && targetm.modes_tieable_p (read_mode, store_mode))
+ && targetm.modes_tieable_p (read_mode, store_mode)
+ /* It's invalid in validate_subreg if read_mode size is < reg natural. */
+ && known_ge (GET_MODE_SIZE (read_mode), REGMODE_NATURAL_SIZE (read_mode)))
read_reg = gen_lowpart (read_mode, copy_rtx (store_info->rhs));
else
read_reg = extract_low_bits (read_mode, store_mode,
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;
+}
diff --git a/gcc/testsuite/gcc.target/riscv/rvv/base/pr111720-10.c b/gcc/testsuite/gcc.target/riscv/rvv/base/pr111720-10.c
index 215eb99ce0f..ee6b2ccf7ad 100644
--- a/gcc/testsuite/gcc.target/riscv/rvv/base/pr111720-10.c
+++ b/gcc/testsuite/gcc.target/riscv/rvv/base/pr111720-10.c
@@ -15,4 +15,4 @@ vbool4_t test () {
}
/* { dg-final { scan-assembler-not {vle[0-9]+\.v\s+v[0-9]+,\s*[0-9]+\(sp\)} } } */
-/* { dg-final { scan-assembler-not {vs[0-9]+r\.v\s+v[0-9]+,\s*[0-9]+\(sp\)} } } */
+/* { dg-final { scan-assembler-times {vs[0-9]+r\.v\s+v[0-9]+,\s*[0-9]+\(sp\)} 1 } } */
--
2.34.1
^ permalink raw reply [flat|nested] 31+ messages in thread
* RE: [PATCH v4] DSE: Fix ICE after allow vector type in get_stored_val
2024-05-03 1:51 ` [PATCH v4] " pan2.li
@ 2024-05-16 4:06 ` Li, Pan2
0 siblings, 0 replies; 31+ messages in thread
From: Li, Pan2 @ 2024-05-16 4:06 UTC (permalink / raw)
To: gcc-patches
Cc: jeffreyalaw, juzhe.zhong, kito.cheng, Liu, Hongtao, richard.guenther
Kindly ping, looks no build error from Linaro for arm.
Pan
-----Original Message-----
From: Li, Pan2 <pan2.li@intel.com>
Sent: Friday, May 3, 2024 9:52 AM
To: gcc-patches@gcc.gnu.org
Cc: jeffreyalaw@gmail.com; juzhe.zhong@rivai.ai; kito.cheng@gmail.com; Liu, Hongtao <hongtao.liu@intel.com>; richard.guenther@gmail.com; Li, Pan2 <pan2.li@intel.com>
Subject: [PATCH v4] DSE: Fix ICE after allow vector type in get_stored_val
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, the valididate_subreg
treats the vector type's size is less than vector register as
invalid. Then we will have ICE here.
This patch would like to fix it by filter-out the invalid type size,
and make sure the subreg is valid for both the read_mode and store_mode
before perform the real gen_lowpart.
The below test suites are passed for this patch:
* The x86 bootstrap test.
* The x86 regression test.
* The riscv rv64gcv regression test.
* The riscv rv64gc regression test.
* The aarch64 regression test.
gcc/ChangeLog:
* dse.cc (get_stored_val): Make sure read_mode/write_mode
is valid subreg before gen_lowpart.
gcc/testsuite/ChangeLog:
* gcc.target/riscv/rvv/base/bug-6.c: New test.
Signed-off-by: Pan Li <pan2.li@intel.com>
---
gcc/dse.cc | 4 +++-
.../gcc.target/riscv/rvv/base/bug-6.c | 22 +++++++++++++++++++
2 files changed, 25 insertions(+), 1 deletion(-)
create mode 100644 gcc/testsuite/gcc.target/riscv/rvv/base/bug-6.c
diff --git a/gcc/dse.cc b/gcc/dse.cc
index edc7a1dfecf..1596da91da0 100644
--- a/gcc/dse.cc
+++ b/gcc/dse.cc
@@ -1946,7 +1946,9 @@ get_stored_val (store_info *store_info, machine_mode read_mode,
copy_rtx (store_info->const_rhs));
else if (VECTOR_MODE_P (read_mode) && VECTOR_MODE_P (store_mode)
&& known_le (GET_MODE_BITSIZE (read_mode), GET_MODE_BITSIZE (store_mode))
- && targetm.modes_tieable_p (read_mode, store_mode))
+ && targetm.modes_tieable_p (read_mode, store_mode)
+ && validate_subreg (read_mode, store_mode, copy_rtx (store_info->rhs),
+ subreg_lowpart_offset (read_mode, store_mode)))
read_reg = gen_lowpart (read_mode, copy_rtx (store_info->rhs));
else
read_reg = extract_low_bits (read_mode, store_mode,
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
^ permalink raw reply [flat|nested] 31+ messages in thread
end of thread, other threads:[~2024-05-16 4:06 UTC | newest]
Thread overview: 31+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-02-26 3:25 [PATCH v1] RTL: Bugfix ICE after allow vector type in DSE pan2.li
2024-02-26 3:41 ` Hongtao Liu
2024-02-26 3:42 ` Li, Pan2
2024-02-26 5:13 ` Hongtao Liu
2024-02-26 7:38 ` Richard Biener
2024-02-26 7:41 ` Li, Pan2
2024-02-26 14:22 ` [PATCH v2] DSE: Bugfix ICE after allow vector type in get_stored_val pan2.li
2024-02-27 9:47 ` Richard Biener
2024-02-27 15:02 ` Jeff Law
2024-02-28 1:40 ` Li, Pan2
2024-02-28 4:51 ` Li, Pan2
2024-02-28 17:33 ` Jeff Law
2024-02-29 1:38 ` Li, Pan2
2024-02-29 13:28 ` Robin Dapp
2024-03-02 1:04 ` Li, Pan2
2024-03-03 22:46 ` Jeff Law
2024-03-05 6:22 ` Li, Pan2
2024-03-12 2:08 ` Li, Pan2
2024-03-22 1:15 ` Li, Pan2
2024-03-22 18:53 ` Jeff Law
2024-03-23 5:45 ` Li, Pan2
2024-04-06 12:02 ` Li, Pan2
2024-04-18 1:46 ` Li, Pan2
2024-04-28 12:10 ` Li, Pan2
2024-04-29 15:20 ` Jeff Law
2024-04-30 1:02 ` Li, Pan2
2024-04-30 7:17 ` [PATCH v3] DSE: Fix " pan2.li
2024-04-30 11:35 ` Li, Pan2
2024-05-03 1:57 ` Li, Pan2
2024-05-03 1:51 ` [PATCH v4] " pan2.li
2024-05-16 4:06 ` Li, Pan2
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).