diff mbox series

Improve reg_or_subregno to return INVALID_REGNUM when the subreg of memory is processed.

Message ID 20220623033916.32217-1-hongtao.liu@intel.com
State New
Headers show
Series Improve reg_or_subregno to return INVALID_REGNUM when the subreg of memory is processed. | expand

Commit Message

liuhongt June 23, 2022, 3:39 a.m. UTC 
Hi:
  This is follow-up to [1], return INVALID_REGNUM instead of gcc_assert,
  also adjust some conditions guarded for reg_or_subregno.

>OK, but I think that reg_or_subregno should be improved to return
>INVALID_REGNUM when the subreg of memory is processed.
>
>Thanks,
>Uros.

[1] https://gcc.gnu.org/pipermail/gcc-patches/2022-June/596841.html

Bootstrapped and regtested on x86_64-pc-linux{-m32,}.
Ok for trunk?

gcc/ChangeLog:

	* combine.cc (try_combine): Replace condition with regno !=
	INVALID_REGNUM.
	* jump.cc (reg_or_subregno): Remove condition for (REG_P (x) ||
	(SUBREG_P (x) && REG_P (SUBREG_REG (x)))).
	* reload.cc (push_reload): Ditto.
---
 gcc/combine.cc | 7 ++-----
 gcc/jump.cc    | 4 ++--
 gcc/reload.cc  | 4 ----
 3 files changed, 4 insertions(+), 11 deletions(-)

Comments

Segher Boessenkool June 24, 2022, 4:40 p.m. UTC | #1 
Hi!

Please Cc: me on all combine patches, so that I will not miss any.

On Thu, Jun 23, 2022 at 11:39:16AM +0800, liuhongt via Gcc-patches wrote:
>   This is follow-up to [1], return INVALID_REGNUM instead of gcc_assert,
>   also adjust some conditions guarded for reg_or_subregno.
> 
> >OK, but I think that reg_or_subregno should be improved to return
> >INVALID_REGNUM when the subreg of memory is processed.

> 	* combine.cc (try_combine): Replace condition with regno !=
> 	INVALID_REGNUM.

Not okay.

> --- a/gcc/combine.cc
> +++ b/gcc/combine.cc
> @@ -4245,12 +4245,9 @@ try_combine (rtx_insn *i3, rtx_insn *i2, rtx_insn *i1, rtx_insn *i0,
>        if (GET_CODE (x) == PARALLEL)
>  	x = XVECEXP (newi2pat, 0, 0);
>  
> -      if (REG_P (SET_DEST (x))
> -	  || (GET_CODE (SET_DEST (x)) == SUBREG
> -	      && REG_P (SUBREG_REG (SET_DEST (x)))))
> +      unsigned int regno = reg_or_subregno (SET_DEST (x));
> +      if (regno != INVALID_REGNUM)
>  	{
> -	  unsigned int regno = reg_or_subregno (SET_DEST (x));

This is at best obfuscation :-(

Subregs of non-reg have been deprecated for ages, and will be completely
phased out soon, and this patch goes in the wrong direction.  But also,
this change is obfuscation, in multiple ways.  Firstly, using a regno to
mean something is not a register at all, but worse, you use a number
that is a valid regnum (but too big to happen in practice of course) to
indicate this.  While in other places the negative -1 is used, which is
a bit more obvious.

>  	  && reg_or_subregno (in) < FIRST_PSEUDO_REGISTER)

... and then statements like this will implicitly do something for
subregs of memory, which is totally not obvious.

The combine pass still has explicit support for subregs of memory, and
the support for subregs is one of the more error-prone and non-obvious
parts of combine in the first place.  Anything that makes things harder
here will not be approved.


Segher
diff mbox series

Patch

diff --git a/gcc/combine.cc b/gcc/combine.cc
index 9a34ef847aa..3e6feb323f7 100644
--- a/gcc/combine.cc
+++ b/gcc/combine.cc
@@ -4245,12 +4245,9 @@  try_combine (rtx_insn *i3, rtx_insn *i2, rtx_insn *i1, rtx_insn *i0,
       if (GET_CODE (x) == PARALLEL)
 	x = XVECEXP (newi2pat, 0, 0);
 
-      if (REG_P (SET_DEST (x))
-	  || (GET_CODE (SET_DEST (x)) == SUBREG
-	      && REG_P (SUBREG_REG (SET_DEST (x)))))
+      unsigned int regno = reg_or_subregno (SET_DEST (x));
+      if (regno != INVALID_REGNUM)
 	{
-	  unsigned int regno = reg_or_subregno (SET_DEST (x));
-
 	  bool done = false;
 	  for (rtx_insn *insn = NEXT_INSN (i3);
 	       !done
diff --git a/gcc/jump.cc b/gcc/jump.cc
index 332f86878e1..dd8fb50e08e 100644
--- a/gcc/jump.cc
+++ b/gcc/jump.cc
@@ -1889,6 +1889,6 @@  reg_or_subregno (const_rtx reg)
 {
   if (GET_CODE (reg) == SUBREG)
     reg = SUBREG_REG (reg);
-  gcc_assert (REG_P (reg));
-  return REGNO (reg);
+
+  return REG_P (reg) ? REGNO (reg) : INVALID_REGNUM;
 }
diff --git a/gcc/reload.cc b/gcc/reload.cc
index 3ed901e3944..c7f4fc9d965 100644
--- a/gcc/reload.cc
+++ b/gcc/reload.cc
@@ -1369,8 +1369,6 @@  push_reload (rtx in, rtx out, rtx *inloc, rtx *outloc,
 
       if (subreg_in_class == NO_REGS
 	  && in != 0
-	  && (REG_P (in)
-	      || (GET_CODE (in) == SUBREG && REG_P (SUBREG_REG (in))))
 	  && reg_or_subregno (in) < FIRST_PSEUDO_REGISTER)
 	subreg_in_class = REGNO_REG_CLASS (reg_or_subregno (in));
       /* If a memory location is needed for the copy, make one.  */
@@ -1401,8 +1399,6 @@  push_reload (rtx in, rtx out, rtx *inloc, rtx *outloc,
       n_reloads++;
 
       if (out != 0
-          && (REG_P (out)
-	      || (GET_CODE (out) == SUBREG && REG_P (SUBREG_REG (out))))
 	  && reg_or_subregno (out) < FIRST_PSEUDO_REGISTER
 	  && (targetm.secondary_memory_needed
 	      (outmode, rclass, REGNO_REG_CLASS (reg_or_subregno (out)))))