diff mbox series

reload: Adjust find_reloads to comment; test intersection, not subset

Message ID 20220201014140.A2D9220419@pchp3.se.axis.com
State New
Headers show
Series reload: Adjust find_reloads to comment; test intersection, not subset | expand

Commit Message

Hans-Peter Nilsson Feb. 1, 2022, 1:41 a.m. UTC 
I'm not seriously submitting this patch for approval.  I just thought
it'd be interesting to some people, at least those maintaining ports
still using reload; I know it's reload and major ports don't really
care about that anymore.  TL;DR: scroll down for the mystery part.

The *benevolent* effects of this bug had me confused for a while: I
thought I'd see neutral or good effects of nominally *improving* the
register-class topology of the CRIS port such that there's "a class
for the union of two classes whenever some instruction allows both
classes".  Essentially, IRA picked (still does, not committed yet) two
classes GENNONACR_REGS and SPECIAL_REGS and SPEC_GENNONACR_REGS, their
union, but the move-pattern alternatives corresponding to
GENNONACR_REGS are actually "r" (GENERAL_REGS), which also contains
the ACR register.  I want to get rid of all things related to ACR and
simplify the port, as ACR is an artefact of a no-longer-supported CRIS
variant (see commit r11-220 which also mentions this remaining
artefact).

But, when improving the topology, instead of still making good use of
registers from SPECIAL_REGS where the reload pass matters, registers
from GENERAL_REGS are always preferred.  Though, for coremark, this
only happens in two printf-formatting functions of newlib; _vfprintf_r
and _vfiprintf_r (same code actually, and it's likely a bug that both
instances get included).

Anyway, skipping to the point: the effect of the bug is that
alternatives corresponding to classes that are *not subclasses* of the
preferred class (the union), are demoted by 4 (or actually: 2 + 2 *
pref_or_nothing[i]), instead of those register alternatives where the
register-class *intersection is empty*, as is the intent according to
my reading of the comment.  For CRIS, this means that GENERAL_REGS get
demoted (as ACR is present there but not in SPEC_GENNONACR_REGS) and
then SPECIAL_REGS are preferred, which is *sometimes* better as those
registers are "free".  They're not used in regular code and outside
their special purposes, basically just usable for moving around data;
perfect for plain single-use spill-reloads.  Net result: for CRIS the
bug causes a slightly smaller coremark binary but the same execution
time.

With the CRIS register-class topology fixed and the bug no longer
having effect, I have to adjust the port such that SPECIAL_REGS are
preferred when that helps (some heuristics required), as the (current)
order of alternatives for the *movsi_internal pattern causes
preference of GENERAL_REGS over SPECIAL_REGS when the cost is the
same.  IOW, a flaw in the port masked by a bug in reload.

The mystery isn't so much that there's code mismatching comments or
intent, but that this code has been there "forever".  There has been a
function reg_classes_intersect_p, in gcc since r0-54, even *before*
there was reload.c; r0-361, so why the "we don't have a way of forming
the intersection" in the actual patch and why wasn't this fixed
(perhaps not even noticed) when reload was state-of-the-art?

(I know the file has been renamed but again this isn't an patch
intended to be applied.  The context is adjusted to include the
comment.  The ChangeLog entry is the effect of a habitual twitch. 8-]
)

gcc:
	* reload.c (find_reloads): Call reg_classes_intersect_p, not
	reg_class_subset_p.
---
 gcc/reload.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

Comments

Richard Sandiford Feb. 1, 2022, 3:57 a.m. UTC | #1 
Hans-Peter Nilsson via Gcc-patches <gcc-patches@gcc.gnu.org> writes:
> I'm not seriously submitting this patch for approval.  I just thought
> it'd be interesting to some people, at least those maintaining ports
> still using reload; I know it's reload and major ports don't really
> care about that anymore.  TL;DR: scroll down for the mystery part.
>
> The *benevolent* effects of this bug had me confused for a while: I
> thought I'd see neutral or good effects of nominally *improving* the
> register-class topology of the CRIS port such that there's "a class
> for the union of two classes whenever some instruction allows both
> classes".  Essentially, IRA picked (still does, not committed yet) two
> classes GENNONACR_REGS and SPECIAL_REGS and SPEC_GENNONACR_REGS, their
> union, but the move-pattern alternatives corresponding to
> GENNONACR_REGS are actually "r" (GENERAL_REGS), which also contains
> the ACR register.  I want to get rid of all things related to ACR and
> simplify the port, as ACR is an artefact of a no-longer-supported CRIS
> variant (see commit r11-220 which also mentions this remaining
> artefact).
>
> But, when improving the topology, instead of still making good use of
> registers from SPECIAL_REGS where the reload pass matters, registers
> from GENERAL_REGS are always preferred.  Though, for coremark, this
> only happens in two printf-formatting functions of newlib; _vfprintf_r
> and _vfiprintf_r (same code actually, and it's likely a bug that both
> instances get included).
>
> Anyway, skipping to the point: the effect of the bug is that
> alternatives corresponding to classes that are *not subclasses* of the
> preferred class (the union), are demoted by 4 (or actually: 2 + 2 *
> pref_or_nothing[i]), instead of those register alternatives where the
> register-class *intersection is empty*, as is the intent according to
> my reading of the comment.  For CRIS, this means that GENERAL_REGS get
> demoted (as ACR is present there but not in SPEC_GENNONACR_REGS) and
> then SPECIAL_REGS are preferred, which is *sometimes* better as those
> registers are "free".  They're not used in regular code and outside
> their special purposes, basically just usable for moving around data;
> perfect for plain single-use spill-reloads.  Net result: for CRIS the
> bug causes a slightly smaller coremark binary but the same execution
> time.
>
> With the CRIS register-class topology fixed and the bug no longer
> having effect, I have to adjust the port such that SPECIAL_REGS are
> preferred when that helps (some heuristics required), as the (current)
> order of alternatives for the *movsi_internal pattern causes
> preference of GENERAL_REGS over SPECIAL_REGS when the cost is the
> same.  IOW, a flaw in the port masked by a bug in reload.
>
> The mystery isn't so much that there's code mismatching comments or
> intent, but that this code has been there "forever".  There has been a
> function reg_classes_intersect_p, in gcc since r0-54, even *before*
> there was reload.c; r0-361, so why the "we don't have a way of forming
> the intersection" in the actual patch and why wasn't this fixed
> (perhaps not even noticed) when reload was state-of-the-art?

But doesn't the comment mean that we have/had no way of getting
a *class* that is the intersection of preferred_class[i] and
this_alternative[i], to store as the new this_alternative[i]?
If the alternatives were register sets rather than classes,
I think the intended effect would be:

  this_alternative[i] &= preferred_class[i];

(i.e. AND_HARD_REG_SET in old money).

It looks like the patch would instead make this_alternative[i] include
registers that the alternative doesn't actually accept, which feels odd.

Thanks,
Richard

> (I know the file has been renamed but again this isn't an patch
> intended to be applied.  The context is adjusted to include the
> comment.  The ChangeLog entry is the effect of a habitual twitch. 8-]
> )
>
> gcc:
> 	* reload.c (find_reloads): Call reg_classes_intersect_p, not
> 	reg_class_subset_p.
> ---
>  gcc/reload.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/gcc/reload.c b/gcc/reload.c
> index 4c55ca58a5f7..36e887c6b57e 100644
> --- a/gcc/reload.c
> +++ b/gcc/reload.c
> @@ -3616,43 +3616,39 @@ find_reloads (rtx_insn *insn, int replace, int ind_levels, int live_known,
>  	      /* If this operand is a pseudo register that didn't get
>  		 a hard reg and this alternative accepts some
>  		 register, see if the class that we want is a subset
>  		 of the preferred class for this register.  If not,
>  		 but it intersects that class, use the preferred class
>  		 instead.  If it does not intersect the preferred
>  		 class, show that usage of this alternative should be
>  		 discouraged; it will be discouraged more still if the
>  		 register is `preferred or nothing'.  We do this
>  		 because it increases the chance of reusing our spill
>  		 register in a later insn and avoiding a pair of
>  		 memory stores and loads.
>  
>  		 Don't bother with this if this alternative will
>  		 accept this operand.
>  
>  		 Don't do this for a multiword operand, since it is
>  		 only a small win and has the risk of requiring more
>  		 spill registers, which could cause a large loss.
>  
>  		 Don't do this if the preferred class has only one
>  		 register because we might otherwise exhaust the
>  		 class.  */
>  
>  	      if (! win && ! did_match
>  		  && this_alternative[i] != NO_REGS
>  		  && known_le (GET_MODE_SIZE (operand_mode[i]), UNITS_PER_WORD)
>  		  && reg_class_size [(int) preferred_class[i]] > 0
>  		  && ! small_register_class_p (preferred_class[i]))
>  		{
>  		  if (! reg_class_subset_p (this_alternative[i],
>  					    preferred_class[i]))
>  		    {
> -		      /* Since we don't have a way of forming the intersection,
> -			 we just do something special if the preferred class
> -			 is a subset of the class we have; that's the most
> -			 common case anyway.  */
> -		      if (reg_class_subset_p (preferred_class[i],
> -					      this_alternative[i]))
> +		      if (reg_classes_intersect_p (preferred_class[i],
> +						   this_alternative[i]))
>  			this_alternative[i] = preferred_class[i];
>  		      else
>  			reject += (2 + 2 * pref_or_nothing[i]);
>  		    }
diff mbox series

Patch

diff --git a/gcc/reload.c b/gcc/reload.c
index 4c55ca58a5f7..36e887c6b57e 100644
--- a/gcc/reload.c
+++ b/gcc/reload.c
@@ -3616,43 +3616,39 @@  find_reloads (rtx_insn *insn, int replace, int ind_levels, int live_known,
 	      /* If this operand is a pseudo register that didn't get
 		 a hard reg and this alternative accepts some
 		 register, see if the class that we want is a subset
 		 of the preferred class for this register.  If not,
 		 but it intersects that class, use the preferred class
 		 instead.  If it does not intersect the preferred
 		 class, show that usage of this alternative should be
 		 discouraged; it will be discouraged more still if the
 		 register is `preferred or nothing'.  We do this
 		 because it increases the chance of reusing our spill
 		 register in a later insn and avoiding a pair of
 		 memory stores and loads.
 
 		 Don't bother with this if this alternative will
 		 accept this operand.
 
 		 Don't do this for a multiword operand, since it is
 		 only a small win and has the risk of requiring more
 		 spill registers, which could cause a large loss.
 
 		 Don't do this if the preferred class has only one
 		 register because we might otherwise exhaust the
 		 class.  */
 
 	      if (! win && ! did_match
 		  && this_alternative[i] != NO_REGS
 		  && known_le (GET_MODE_SIZE (operand_mode[i]), UNITS_PER_WORD)
 		  && reg_class_size [(int) preferred_class[i]] > 0
 		  && ! small_register_class_p (preferred_class[i]))
 		{
 		  if (! reg_class_subset_p (this_alternative[i],
 					    preferred_class[i]))
 		    {
-		      /* Since we don't have a way of forming the intersection,
-			 we just do something special if the preferred class
-			 is a subset of the class we have; that's the most
-			 common case anyway.  */
-		      if (reg_class_subset_p (preferred_class[i],
-					      this_alternative[i]))
+		      if (reg_classes_intersect_p (preferred_class[i],
+						   this_alternative[i]))
 			this_alternative[i] = preferred_class[i];
 		      else
 			reject += (2 + 2 * pref_or_nothing[i]);
 		    }