diff mbox series

[2/2] Get rid of all float-int special cases in validate_subreg.

Message ID 20210831111749.1530591-3-hongtao.liu@intel.com
State New
Headers show
Series Get rid of all float-int special cases in validate_subreg. | expand

Commit Message

liuhongt Aug. 31, 2021, 11:17 a.m. UTC 
gcc/ChangeLog:

	* emit-rtl.c (validate_subreg): Get rid of all float-int
	special cases.
---
 gcc/emit-rtl.c | 40 ----------------------------------------
 1 file changed, 40 deletions(-)

Comments

Richard Biener Aug. 31, 2021, 11:57 a.m. UTC | #1 
On Tue, Aug 31, 2021 at 1:17 PM liuhongt <hongtao.liu@intel.com> wrote:
>
> gcc/ChangeLog:

OK.

Thanks,
Richard.

>         * emit-rtl.c (validate_subreg): Get rid of all float-int
>         special cases.
> ---
>  gcc/emit-rtl.c | 40 ----------------------------------------
>  1 file changed, 40 deletions(-)
>
> diff --git a/gcc/emit-rtl.c b/gcc/emit-rtl.c
> index ff3b4449b37..77ea8948ee8 100644
> --- a/gcc/emit-rtl.c
> +++ b/gcc/emit-rtl.c
> @@ -922,46 +922,6 @@ validate_subreg (machine_mode omode, machine_mode imode,
>
>    poly_uint64 regsize = REGMODE_NATURAL_SIZE (imode);
>
> -  /* ??? This should not be here.  Temporarily continue to allow word_mode
> -     subregs of anything.  The most common offender is (subreg:SI (reg:DF)).
> -     Generally, backends are doing something sketchy but it'll take time to
> -     fix them all.  */
> -  if (omode == word_mode)
> -    ;
> -  /* ??? 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))
> -    ;
> -  /* Allow component subregs of complex and vector.  Though given the below
> -     extraction rules, it's not always clear what that means.  */
> -  else if ((COMPLEX_MODE_P (imode) || VECTOR_MODE_P (imode))
> -          && GET_MODE_INNER (imode) == omode)
> -    ;
> -  /* ??? x86 sse code makes heavy use of *paradoxical* vector subregs,
> -     i.e. (subreg:V4SF (reg:SF) 0) or (subreg:V4SF (reg:V2SF) 0).  This
> -     surely isn't the cleanest way to represent this.  It's questionable
> -     if this ought to be represented at all -- why can't this all be hidden
> -     in post-reload splitters that make arbitrarily mode changes to the
> -     registers themselves.  */
> -  else if (VECTOR_MODE_P (omode)
> -          && GET_MODE_INNER (omode) == GET_MODE_INNER (imode))
> -    ;
> -  /* Subregs involving floating point modes are not allowed to
> -     change size.  Therefore (subreg:DI (reg:DF) 0) is fine, but
> -     (subreg:SI (reg:DF) 0) isn't.  */
> -  else if (FLOAT_MODE_P (imode) || FLOAT_MODE_P (omode))
> -    {
> -      if (! (known_eq (isize, osize)
> -            /* LRA can use subreg to store a floating point value in
> -               an integer mode.  Although the floating point and the
> -               integer modes need the same number of hard registers,
> -               the size of floating point mode can be less than the
> -               integer mode.  LRA also uses subregs for a register
> -               should be used in different mode in on insn.  */
> -            || lra_in_progress))
> -       return false;
> -    }
> -
>    /* Paradoxical subregs must have offset zero.  */
>    if (maybe_gt (osize, isize))
>      return known_eq (offset, 0U);
> --
> 2.27.0
>
Segher Boessenkool Sept. 2, 2021, 5:55 p.m. UTC | #2 
On Tue, Aug 31, 2021 at 07:17:49PM +0800, liuhongt via Gcc-patches wrote:
> 	* emit-rtl.c (validate_subreg): Get rid of all float-int
> 	special cases.

This caused various regressions on powerpc.  Please revert this until
this can be done safely (the comment this patch deletes says why it can
not be done yet).


Segher
Andreas Schwab Sept. 3, 2021, 3:05 p.m. UTC | #3 
On Sep 02 2021, Segher Boessenkool wrote:

> On Tue, Aug 31, 2021 at 07:17:49PM +0800, liuhongt via Gcc-patches wrote:
>> 	* emit-rtl.c (validate_subreg): Get rid of all float-int
>> 	special cases.
>
> This caused various regressions on powerpc.  Please revert this until
> this can be done safely (the comment this patch deletes says why it can
> not be done yet).

This also breaks ada on riscv64.

s-fatgen.adb: In function 'System.Fat_Flt.Attr_Float.Scaling':
s-fatgen.adb:830:8: error: unable to find a register to spill
s-fatgen.adb:830:8: error: this is the insn:
(insn 215 321 216 26 (set (reg:SF 88 [ xx.26_39 ])
        (mult:SF (reg:SF 190)
            (subreg:SF (reg:DI 221 [164]) 0))) "s-fatgen.adb":821:25 17 {mulsf3}
     (expr_list:REG_DEAD (reg:DI 221 [164])
        (expr_list:REG_DEAD (reg:SF 190)
            (nil))))
during RTL pass: reload

Andreas.
Segher Boessenkool Sept. 7, 2021, 11:19 p.m. UTC | #4 
On Fri, Sep 03, 2021 at 05:05:47PM +0200, Andreas Schwab wrote:
> On Sep 02 2021, Segher Boessenkool wrote:
> > On Tue, Aug 31, 2021 at 07:17:49PM +0800, liuhongt via Gcc-patches wrote:
> >> 	* emit-rtl.c (validate_subreg): Get rid of all float-int
> >> 	special cases.
> >
> > This caused various regressions on powerpc.  Please revert this until
> > this can be done safely (the comment this patch deletes says why it can
> > not be done yet).
> 
> This also breaks ada on riscv64.
> 
> s-fatgen.adb: In function 'System.Fat_Flt.Attr_Float.Scaling':
> s-fatgen.adb:830:8: error: unable to find a register to spill
> s-fatgen.adb:830:8: error: this is the insn:
> (insn 215 321 216 26 (set (reg:SF 88 [ xx.26_39 ])
>         (mult:SF (reg:SF 190)
>             (subreg:SF (reg:DI 221 [164]) 0))) "s-fatgen.adb":821:25 17 {mulsf3}
>      (expr_list:REG_DEAD (reg:DI 221 [164])
>         (expr_list:REG_DEAD (reg:SF 190)
>             (nil))))
> during RTL pass: reload

It still is broken on rs6000.  This breaks when building SPEC for
example (but in many more places as well).

This needs to be fixed somehow.

I sent <https://gcc.gnu.org/pipermail/gcc-patches/2021-September/579026.html>
(Message-ID: <20210907230730.GM1583@gate.crashing.org>) that may be a
start discussing this somewhat.  The idea of the change looks fine, but
the time isn't ripe for it yet (if it was intentional!)

In the meantime, various targets still are broken.  This needs a real
fix.  How many *other* targets have been broken, just not detected yet?


Segher
Hongtao Liu Sept. 8, 2021, 12:55 a.m. UTC | #5 
On Wed, Sep 8, 2021 at 7:20 AM Segher Boessenkool
<segher@kernel.crashing.org> wrote:
>
> On Fri, Sep 03, 2021 at 05:05:47PM +0200, Andreas Schwab wrote:
> > On Sep 02 2021, Segher Boessenkool wrote:
> > > On Tue, Aug 31, 2021 at 07:17:49PM +0800, liuhongt via Gcc-patches wrote:
> > >>    * emit-rtl.c (validate_subreg): Get rid of all float-int
> > >>    special cases.
> > >
> > > This caused various regressions on powerpc.  Please revert this until
> > > this can be done safely (the comment this patch deletes says why it can
> > > not be done yet).
> >
> > This also breaks ada on riscv64.
> >
> > s-fatgen.adb: In function 'System.Fat_Flt.Attr_Float.Scaling':
> > s-fatgen.adb:830:8: error: unable to find a register to spill
> > s-fatgen.adb:830:8: error: this is the insn:
> > (insn 215 321 216 26 (set (reg:SF 88 [ xx.26_39 ])
> >         (mult:SF (reg:SF 190)
> >             (subreg:SF (reg:DI 221 [164]) 0))) "s-fatgen.adb":821:25 17 {mulsf3}
> >      (expr_list:REG_DEAD (reg:DI 221 [164])
> >         (expr_list:REG_DEAD (reg:SF 190)
> >             (nil))))
> > during RTL pass: reload
>
> It still is broken on rs6000.  This breaks when building SPEC for
> example (but in many more places as well).
>
> This needs to be fixed somehow.
>
> I sent <https://gcc.gnu.org/pipermail/gcc-patches/2021-September/579026.html>
> (Message-ID: <20210907230730.GM1583@gate.crashing.org>) that may be a
> start discussing this somewhat.  The idea of the change looks fine, but
> the time isn't ripe for it yet (if it was intentional!)
>
> In the meantime, various targets still are broken.  This needs a real
> fix.  How many *other* targets have been broken, just not detected yet?
riscv64 report related bug.
Other than that, no other target reports related regression yet.
>
>
> Segher
diff mbox series

Patch

diff --git a/gcc/emit-rtl.c b/gcc/emit-rtl.c
index ff3b4449b37..77ea8948ee8 100644
--- a/gcc/emit-rtl.c
+++ b/gcc/emit-rtl.c
@@ -922,46 +922,6 @@  validate_subreg (machine_mode omode, machine_mode imode,
 
   poly_uint64 regsize = REGMODE_NATURAL_SIZE (imode);
 
-  /* ??? This should not be here.  Temporarily continue to allow word_mode
-     subregs of anything.  The most common offender is (subreg:SI (reg:DF)).
-     Generally, backends are doing something sketchy but it'll take time to
-     fix them all.  */
-  if (omode == word_mode)
-    ;
-  /* ??? 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))
-    ;
-  /* Allow component subregs of complex and vector.  Though given the below
-     extraction rules, it's not always clear what that means.  */
-  else if ((COMPLEX_MODE_P (imode) || VECTOR_MODE_P (imode))
-	   && GET_MODE_INNER (imode) == omode)
-    ;
-  /* ??? x86 sse code makes heavy use of *paradoxical* vector subregs,
-     i.e. (subreg:V4SF (reg:SF) 0) or (subreg:V4SF (reg:V2SF) 0).  This
-     surely isn't the cleanest way to represent this.  It's questionable
-     if this ought to be represented at all -- why can't this all be hidden
-     in post-reload splitters that make arbitrarily mode changes to the
-     registers themselves.  */
-  else if (VECTOR_MODE_P (omode)
-	   && GET_MODE_INNER (omode) == GET_MODE_INNER (imode))
-    ;
-  /* Subregs involving floating point modes are not allowed to
-     change size.  Therefore (subreg:DI (reg:DF) 0) is fine, but
-     (subreg:SI (reg:DF) 0) isn't.  */
-  else if (FLOAT_MODE_P (imode) || FLOAT_MODE_P (omode))
-    {
-      if (! (known_eq (isize, osize)
-	     /* LRA can use subreg to store a floating point value in
-		an integer mode.  Although the floating point and the
-		integer modes need the same number of hard registers,
-		the size of floating point mode can be less than the
-		integer mode.  LRA also uses subregs for a register
-		should be used in different mode in on insn.  */
-	     || lra_in_progress))
-	return false;
-    }
-
   /* Paradoxical subregs must have offset zero.  */
   if (maybe_gt (osize, isize))
     return known_eq (offset, 0U);