diff mbox series

PR91068: Fix MIPS fallout from IRA matched operand changes

Message ID mptr274zwwh.fsf@arm.com
State New
Headers show
Series PR91068: Fix MIPS fallout from IRA matched operand changes | expand

Commit Message

Richard Sandiford July 5, 2019, 8:51 a.m. UTC
PR91068 is a case in which we have (ignoring non-LRA alternatives):

  [(set (match_operand:SI 0 "register_operand" "=l,d?")
	(plus:SI (mult:SI (match_operand:SI 1 "register_operand" "d,d")
			  (match_operand:SI 2 "register_operand" "d,d"))
		 (match_operand:SI 3 "register_operand" "0,d")))
   (clobber (match_scratch:SI 4 "=X,l"))
   (clobber (match_scratch:SI 5 "=X,&d"))]

where the first alternative is one instruction but the second is two.
This is very similar to the case that my recent IRA patches were
supposed to help.  The crucial difference is that the cheap
alternative requires a single-register class while the expensive
alternative uses general registers.

This makes a difference when one of operand 0 or 3 can naturally be
allocated to LO but the other can't.  If IRA makes that allocation,
both alternatives require one reload of equal cost and so the first
alternative clearly wins.

However, if we say that tying operands 0 and 3 saves the cost of a full
move, then all other things being equal, IRA will prefer to allocate
both registers to the same GPR.  The registers will then naturally
fit the second alternative.

This has a more drastic effect in the MIPS case than it should because
using the GPR alternative is much more expensive there than it appears
to the RA.  But that's really a separate problem and something we were
able to live with before my IRA patch.

What makes tying less useful here is the fact that the tied register is
a single-register class.  I think in those circumstances it's better not
to use tied operands at all and instead use "l" for the inputs.
Allocating the input to LO, and allocating the output to LO, then depend
naturally on class costs.  If we decide to allocate at least one of them
to LO, we'll use the cheap alternative, otherwise we'll (correctly) use
the expensive alternative.  This effectively restores the situation
before my IRA patch, but this time making the preference on the input
register more explicit.

I originally wrote the patterns in the early days of IRA, and certainly
well before LRA.  I think they were largely influened by reload rather
than RA proper (see the comment above *mul_acc_si, which is all about
the reload behaviour).  LRA copes with the two-"l" case just fine.

The patch may well cause problems for -mno-lra, but I think we should
cull that option anyway.

Tested on mipsisa64-elf.  OK to install?

Richard


2019-07-05  Richard Sandiford  <richard.sandiford@arm.com>

gcc/
	PR target/91068
	* config/mips/mips.md (*mul_acc_si, *mul_acc_si_r3900, *macc)
	(*msac, *msac_using_macc, *mul_sub_si): Use "l" for input operands
	instead of matching them to "l" output operands.

Comments

Jeff Law July 6, 2019, 4:20 p.m. UTC | #1
On 7/5/19 2:51 AM, Richard Sandiford wrote:
> PR91068 is a case in which we have (ignoring non-LRA alternatives):
> 
>   [(set (match_operand:SI 0 "register_operand" "=l,d?")
> 	(plus:SI (mult:SI (match_operand:SI 1 "register_operand" "d,d")
> 			  (match_operand:SI 2 "register_operand" "d,d"))
> 		 (match_operand:SI 3 "register_operand" "0,d")))
>    (clobber (match_scratch:SI 4 "=X,l"))
>    (clobber (match_scratch:SI 5 "=X,&d"))]
> 
> where the first alternative is one instruction but the second is two.
> This is very similar to the case that my recent IRA patches were
> supposed to help.  The crucial difference is that the cheap
> alternative requires a single-register class while the expensive
> alternative uses general registers.
> 
> This makes a difference when one of operand 0 or 3 can naturally be
> allocated to LO but the other can't.  If IRA makes that allocation,
> both alternatives require one reload of equal cost and so the first
> alternative clearly wins.
> 
> However, if we say that tying operands 0 and 3 saves the cost of a full
> move, then all other things being equal, IRA will prefer to allocate
> both registers to the same GPR.  The registers will then naturally
> fit the second alternative.
> 
> This has a more drastic effect in the MIPS case than it should because
> using the GPR alternative is much more expensive there than it appears
> to the RA.  But that's really a separate problem and something we were
> able to live with before my IRA patch.
> 
> What makes tying less useful here is the fact that the tied register is
> a single-register class.  I think in those circumstances it's better not
> to use tied operands at all and instead use "l" for the inputs.
> Allocating the input to LO, and allocating the output to LO, then depend
> naturally on class costs.  If we decide to allocate at least one of them
> to LO, we'll use the cheap alternative, otherwise we'll (correctly) use
> the expensive alternative.  This effectively restores the situation
> before my IRA patch, but this time making the preference on the input
> register more explicit.
> 
> I originally wrote the patterns in the early days of IRA, and certainly
> well before LRA.  I think they were largely influened by reload rather
> than RA proper (see the comment above *mul_acc_si, which is all about
> the reload behaviour).  LRA copes with the two-"l" case just fine.
> 
> The patch may well cause problems for -mno-lra, but I think we should
> cull that option anyway.
> 
> Tested on mipsisa64-elf.  OK to install?
> 
> Richard
> 
> 
> 2019-07-05  Richard Sandiford  <richard.sandiford@arm.com>
> 
> gcc/
> 	PR target/91068
> 	* config/mips/mips.md (*mul_acc_si, *mul_acc_si_r3900, *macc)
> 	(*msac, *msac_using_macc, *mul_sub_si): Use "l" for input operands
> 	instead of matching them to "l" output operands.
Dredging up memories of past work? :)  I certainly wonder how much crud
we have in the target files that's just not desirable anymore given how
far things have moved.  But I'm also not terribly inclined to spend the
time to figure that out for the embedded targets ;-)



OK.

Jeff
diff mbox series

Patch

Index: gcc/config/mips/mips.md
===================================================================
--- gcc/config/mips/mips.md	2019-07-01 09:37:07.292523884 +0100
+++ gcc/config/mips/mips.md	2019-07-05 09:46:55.219455545 +0100
@@ -1749,7 +1749,7 @@  (define_insn "*mul_acc_si"
   [(set (match_operand:SI 0 "register_operand" "=l*?*?,l,d?")
 	(plus:SI (mult:SI (match_operand:SI 1 "register_operand" "d,d,d")
 			  (match_operand:SI 2 "register_operand" "d,d,d"))
-		 (match_operand:SI 3 "register_operand" "0,0,d")))
+		 (match_operand:SI 3 "register_operand" "l,l,d")))
    (clobber (match_scratch:SI 4 "=X,X,l"))
    (clobber (match_scratch:SI 5 "=X,X,&d"))]
   "GENERATE_MADD_MSUB && !TARGET_MIPS16"
@@ -1778,7 +1778,7 @@  (define_insn "*mul_acc_si_r3900"
   [(set (match_operand:SI 0 "register_operand" "=l*?*?,l,d*?,d?")
 	(plus:SI (mult:SI (match_operand:SI 1 "register_operand" "d,d,d,d")
 			  (match_operand:SI 2 "register_operand" "d,d,d,d"))
-		 (match_operand:SI 3 "register_operand" "0,0,l,d")))
+		 (match_operand:SI 3 "register_operand" "l,l,l,d")))
    (clobber (match_scratch:SI 4 "=X,X,3,l"))
    (clobber (match_scratch:SI 5 "=X,X,X,&d"))]
   "TARGET_MIPS3900 && !TARGET_MIPS16"
@@ -1822,7 +1822,7 @@  (define_insn "*macc"
   [(set (match_operand:SI 0 "register_operand" "=l,d")
 	(plus:SI (mult:SI (match_operand:SI 1 "register_operand" "d,d")
 			  (match_operand:SI 2 "register_operand" "d,d"))
-		 (match_operand:SI 3 "register_operand" "0,l")))
+		 (match_operand:SI 3 "register_operand" "l,l")))
    (clobber (match_scratch:SI 4 "=X,3"))]
   "ISA_HAS_MACC"
 {
@@ -1842,7 +1842,7 @@  (define_insn "*macc"
 
 (define_insn "*msac"
   [(set (match_operand:SI 0 "register_operand" "=l,d")
-        (minus:SI (match_operand:SI 1 "register_operand" "0,l")
+        (minus:SI (match_operand:SI 1 "register_operand" "l,l")
                   (mult:SI (match_operand:SI 2 "register_operand" "d,d")
                            (match_operand:SI 3 "register_operand" "d,d"))))
    (clobber (match_scratch:SI 4 "=X,1"))]
@@ -1862,7 +1862,7 @@  (define_insn "*msac"
 ;; An msac-like instruction implemented using negation and a macc.
 (define_insn_and_split "*msac_using_macc"
   [(set (match_operand:SI 0 "register_operand" "=l,d")
-        (minus:SI (match_operand:SI 1 "register_operand" "0,l")
+        (minus:SI (match_operand:SI 1 "register_operand" "l,l")
                   (mult:SI (match_operand:SI 2 "register_operand" "d,d")
                            (match_operand:SI 3 "register_operand" "d,d"))))
    (clobber (match_scratch:SI 4 "=X,1"))
@@ -2005,7 +2005,7 @@  (define_peephole2
 ;; See the comment above *mul_add_si for details.
 (define_insn "*mul_sub_si"
   [(set (match_operand:SI 0 "register_operand" "=l*?*?,l,d?")
-        (minus:SI (match_operand:SI 1 "register_operand" "0,0,d")
+        (minus:SI (match_operand:SI 1 "register_operand" "l,l,d")
                   (mult:SI (match_operand:SI 2 "register_operand" "d,d,d")
                            (match_operand:SI 3 "register_operand" "d,d,d"))))
    (clobber (match_scratch:SI 4 "=X,X,l"))