Hi Charles,
On 20/11/17 21:09, Charles Baylis wrote:
On 15 September 2017 at 18:01, Kyrill Tkachov
<kyrylo.tkac...@foss.arm.com> wrote:
>
> On 15/09/17 16:38, Charles Baylis wrote:
>>
>> On 13 September 2017 at 10:02, Kyrill Tkachov
>> <kyrylo.tkac...@foss.arm.com> wrote:
>>>
>>> Hi Charles,
>>>
>>> On 12/09/17 09:34, charles.bay...@linaro.org wrote:
>>>>
>>>> From: Charles Baylis <charles.bay...@linaro.org>
>>>>
>>>> This patch moves the calculation of costs for MEM into a
>>>> separate function, and reforms the calculation into two
>>>> parts. Firstly any additional cost of the addressing mode
>>>> is calculated, and then the cost of the memory access itself
>>>> is added.
>>>>
>>>> In this patch, the calculation of the cost of the addressing
>>>> mode is left as a placeholder, to be added in a subsequent
>>>> patch.
>>>>
>>> Can you please mention how has this series been tested?
>>> A bootstrap and test run on arm-none-linux-gnueabihf is required at
>>> least.
>>
>> It has been tested with make check on arm-unknown-linux-gnueabihf with
>> no regressions. I've successfully bootstrapped the next spin.
>
>
> Thanks.
>
>>> Also, do you have any benchmarking results for this?
>>> I agree that generating the addressing modes in the new tests is
>>> desirable.
>>> So I'm not objecting to the goal of this patch, but a check to
make sure
>>> that this doesn't regress SPEC
>>> would be great. Further comments on the patch inline.
>>
>> SPEC2006 scores are unaffected by this patch on Cortex-A57.
>
>
> Good, thanks for checking :)
>
>
>>>> +/* Helper function for arm_rtx_costs_internal. Calculates the
cost of
>>>> a
>>>> MEM,
>>>> + considering the costs of the addressing mode and memory access
>>>> + separately. */
>>>> +static bool
>>>> +arm_mem_costs (rtx x, const struct cpu_cost_table *extra_cost,
>>>> + int *cost, bool speed_p)
>>>> +{
>>>> + machine_mode mode = GET_MODE (x);
>>>> + if (flag_pic
>>>> + && GET_CODE (XEXP (x, 0)) == PLUS
>>>> + && will_be_in_index_register (XEXP (XEXP (x, 0), 1)))
>>>> + /* This will be split into two instructions. Add the cost
of the
>>>> + additional instruction here. The cost of the memory
access is
>>>> computed
>>>> + below. See arm.md:calculate_pic_address. */
>>>> + *cost = COSTS_N_INSNS (1);
>>>> + else
>>>> + *cost = 0;
>>>
>>>
>>> For speed_p we want the size cost of the insn (COSTS_N_INSNS (1) for a
>>> each
>>> insn)
>>> plus the appropriate field in extra_cost. So you should
unconditionally
>>> initialise the cost
>>> to COSTS_N_INSNS (1), conditionally increment it by COSTS_N_INSNS (1)
>>> with
>>> the condition above.
>>
>> OK. I also have to subtract that COSTS_N_INSNS (1) in the if (speed_p)
>> part because the cost of a single bus transfer is included in that
>> initial cost.
>>
>>>> +
>>>> + /* Calculate cost of the addressing mode. */
>>>> + if (speed_p)
>>>> + {
>>>> + /* TODO: Add table-driven costs for addressing modes.
(See patch
>>>> 2) */
>>>> + }
>>>
>>>
>>> You mean "patch 3". I recommend you just remove this conditional from
>>> this
>>> patch and add the logic
>>> in patch 3 entirely.
>>
>> OK.
>>
>>>> +
>>>> + /* Calculate cost of memory access. */
>>>> + if (speed_p)
>>>> + {
>>>> + /* data transfer is transfer size divided by bus width. */
>>>> + int bus_width_bytes = current_tune->bus_width / 4;
>>>
>>>
>>> This should be bus_width / BITS_PER_UNIT to get the size in bytes.
>>> BITS_PER_UNIT is 8 though, so you'll have to double check to make sure
>>> the
>>> cost calculation and generated code is still appropriate.
>>
>> Oops, I changed the units around and messed this up. I'll fix this.
>>
>>>> + *cost += CEIL (GET_MODE_SIZE (mode), bus_width_bytes);
>>>> + *cost += extra_cost->ldst.load;
>>>> + }
>>>> + else
>>>> + {
>>>> + *cost += COSTS_N_INSNS (1);
>>>> + }
>>>
>>> Given my first comment above this else would be deleted.
>>
>> OK
>
>
> I have a concern about using the bus_width parameter which
> I explain in the thread for patch 1 (I don't think we need it, we
should use
> the fields in extra_cost->ldst
> more carefully).
I have modified this patch accordingly. Patch 1 is no longer needed.
Passes "make check" (with patch 3) on arm-linux-gnueabihf with no
regressions. Bootstrap is in progress.
Can I still get this in during stage 3?
Thanks, these are ok for trunk.
They were originally posted way before stage 3 and this is just a rework,
so it's acceptable at this stage as far as I'm concerned.
Thank you for working on these,
Kyrill
gcc/ChangeLog:
<date> Charles Baylis <charles.bay...@linaro.org>
* config/arm/arm.c (arm_mem_costs): New function.
(arm_rtx_costs_internal): Use arm_mem_costs.
gcc/testsuite/ChangeLog:
<date> Charles Baylis <charles.bay...@linaro.org>
* gcc.target/arm/addr-modes-float.c: New test.
* gcc.target/arm/addr-modes-int.c: New test.
* gcc.target/arm/addr-modes.h: New header.
