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).
Kyrill
