* [PATCH][AArch64] Fix aarch64_rtx_costs of PLUS/MINUS
@ 2015-03-04 15:38 Wilco Dijkstra
2015-05-01 10:22 ` Marcus Shawcroft
0 siblings, 1 reply; 6+ messages in thread
From: Wilco Dijkstra @ 2015-03-04 15:38 UTC (permalink / raw)
To: GCC Patches
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 <wdijkstr@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);
+
/* 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH][AArch64] Fix aarch64_rtx_costs of PLUS/MINUS
2015-03-04 15:38 [PATCH][AArch64] Fix aarch64_rtx_costs of PLUS/MINUS Wilco Dijkstra
@ 2015-05-01 10:22 ` Marcus Shawcroft
2015-05-01 12:53 ` Kyrill Tkachov
0 siblings, 1 reply; 6+ messages in thread
From: Marcus Shawcroft @ 2015-05-01 10:22 UTC (permalink / raw)
To: Wilco Dijkstra; +Cc: GCC Patches
On 4 March 2015 at 15:37, Wilco Dijkstra <wdijkstr@arm.com> wrote:
> 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 <wdijkstr@arm.com>
>
> * gcc/config/aarch64/aarch64.c (aarch64_rtx_costs):
> Calculate cost of op0 and op1 in PLUS and MINUS cases.
OK, we take this one instead of Kyrill's. /Marcus
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH][AArch64] Fix aarch64_rtx_costs of PLUS/MINUS
2015-05-01 10:22 ` Marcus Shawcroft
@ 2015-05-01 12:53 ` Kyrill Tkachov
0 siblings, 0 replies; 6+ messages in thread
From: Kyrill Tkachov @ 2015-05-01 12:53 UTC (permalink / raw)
To: Marcus Shawcroft, Wilco Dijkstra; +Cc: GCC Patches
On 01/05/15 11:22, Marcus Shawcroft wrote:
> On 4 March 2015 at 15:37, Wilco Dijkstra <wdijkstr@arm.com> wrote:
>> 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 <wdijkstr@arm.com>
>>
>> * gcc/config/aarch64/aarch64.c (aarch64_rtx_costs):
>> Calculate cost of op0 and op1 in PLUS and MINUS cases.
> OK, we take this one instead of Kyrill's. /Marcus
>
I've committed this patch on Wilcos' behalf with r222676.
Thanks,
Kyrill
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH][AArch64] Fix aarch64_rtx_costs of PLUS/MINUS
2015-04-27 13:53 ` Kyrill Tkachov
@ 2015-04-27 14:17 ` Kyrill Tkachov
0 siblings, 0 replies; 6+ messages in thread
From: Kyrill Tkachov @ 2015-04-27 14:17 UTC (permalink / raw)
To: Wilco Dijkstra, 'GCC Patches'
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:wdijkstr@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 <wdijkstr@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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH][AArch64] Fix aarch64_rtx_costs of PLUS/MINUS
2015-04-27 13:43 Wilco Dijkstra
@ 2015-04-27 13:53 ` Kyrill Tkachov
2015-04-27 14:17 ` Kyrill Tkachov
0 siblings, 1 reply; 6+ messages in thread
From: Kyrill Tkachov @ 2015-04-27 13:53 UTC (permalink / raw)
To: Wilco Dijkstra, 'GCC Patches'
Hi Wilco,
On 27/04/15 14:43, Wilco Dijkstra wrote:
> ping
>
>> -----Original Message-----
>> From: Wilco Dijkstra [mailto:wdijkstr@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 <wdijkstr@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.
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH][AArch64] Fix aarch64_rtx_costs of PLUS/MINUS
@ 2015-04-27 13:43 Wilco Dijkstra
2015-04-27 13:53 ` Kyrill Tkachov
0 siblings, 1 reply; 6+ messages in thread
From: Wilco Dijkstra @ 2015-04-27 13:43 UTC (permalink / raw)
To: 'GCC Patches'
ping
> -----Original Message-----
> From: Wilco Dijkstra [mailto:wdijkstr@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 <wdijkstr@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);
> +
> /* 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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-05-01 12:53 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-03-04 15:38 [PATCH][AArch64] Fix aarch64_rtx_costs of PLUS/MINUS Wilco Dijkstra
2015-05-01 10:22 ` Marcus Shawcroft
2015-05-01 12:53 ` Kyrill Tkachov
2015-04-27 13:43 Wilco Dijkstra
2015-04-27 13:53 ` Kyrill Tkachov
2015-04-27 14:17 ` Kyrill Tkachov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).