On 27/04/15 14:53, Kyrill Tkachov wrote:
Hi Wilco,
On 27/04/15 14:43, Wilco Dijkstra wrote:
ping
-----Original Message-----
From: Wilco Dijkstra [mailto:wdijk...@arm.com]
Sent: 04 March 2015 15:38
To: GCC Patches
Subject: [PATCH][AArch64] Fix aarch64_rtx_costs of PLUS/MINUS
Include the cost of op0 and op1 in all cases in PLUS and MINUS in
aarch64_rtx_costs.
Bootstrap & regression OK.
ChangeLog:
2015-03-04 Wilco Dijkstra <wdijk...@arm.com>
* gcc/config/aarch64/aarch64.c (aarch64_rtx_costs):
Calculate cost of op0 and op1 in PLUS and MINUS cases.
---
gcc/config/aarch64/aarch64.c | 12 +++++-------
1 file changed, 5 insertions(+), 7 deletions(-)
diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index 39921a7..e22d72e 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -5794,6 +5794,8 @@ aarch64_rtx_costs (rtx x, int code, int outer
ATTRIBUTE_UNUSED,
op1 = XEXP (x, 1);
cost_minus:
+ *cost += rtx_cost (op0, MINUS, 0, speed);
+
/* Detect valid immediates. */
if ((GET_MODE_CLASS (mode) == MODE_INT
|| (GET_MODE_CLASS (mode) == MODE_CC
@@ -5801,13 +5803,10 @@ cost_minus:
&& CONST_INT_P (op1)
&& aarch64_uimm12_shift (INTVAL (op1)))
{
- *cost += rtx_cost (op0, MINUS, 0, speed);
-
if (speed)
/* SUB(S) (immediate). */
*cost += extra_cost->alu.arith;
return true;
-
}
/* Look for SUB (extended register). */
@@ -5832,7 +5831,6 @@ cost_minus:
*cost += aarch64_rtx_mult_cost (new_op1, MULT,
(enum rtx_code) code,
speed);
- *cost += rtx_cost (op0, MINUS, 0, speed);
return true;
}
@@ -5879,6 +5877,8 @@ cost_plus:
return true;
}
+ *cost += rtx_cost (op1, PLUS, 1, speed);
+
I don't think this is correct. In the code directly below
(when the aarch64_rtx_arith_op_extract_p condition is true)
we have a shift/extend operation by a constant, so we don't
want to take into account the cost of operand 1 (which is
the extend+shift rtx).
So, I looked at this code myself recently and found that the
wrong logic is addressed by my patch at:
https://gcc.gnu.org/ml/gcc-patches/2015-04/msg01617.html
which fixes the case where we forget to cost operand0 in the
MINUS case.
Never mind, I misread the code. The case is indeed
missing the costing of operand 1, so your patch is
a superset of mine. I think it's correct, but I can't
approve. If this goes in, then I withdraw my patch at:
https://gcc.gnu.org/ml/gcc-patches/2015-04/msg01617.html
Sorry for the confusion,
Kyrill
What do you think?
Kyrill
/* Look for ADD (extended register). */
if (aarch64_rtx_arith_op_extract_p (op0, mode))
{
@@ -5900,12 +5900,10 @@ cost_plus:
{
*cost += aarch64_rtx_mult_cost (new_op0, MULT, PLUS,
speed);
- *cost += rtx_cost (op1, PLUS, 1, speed);
return true;
}
- *cost += (rtx_cost (new_op0, PLUS, 0, speed)
- + rtx_cost (op1, PLUS, 1, speed));
+ *cost += rtx_cost (new_op0, PLUS, 0, speed);
if (speed)
{
--
1.9.1
