This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH 1/3] [ARM] Add bus_width_bits to tune_params
- From: Charles Baylis <charles dot baylis at linaro dot org>
- To: Kyrill Tkachov <kyrylo dot tkachov at foss dot arm dot com>
- Cc: Richard Earnshaw <Richard dot Earnshaw at arm dot com>, Ramana Radhakrishnan <Ramana dot Radhakrishnan at arm dot com>, "pinskia at gmail dot com" <pinskia at gmail dot com>, "gcc-patches at gcc dot gnu dot org" <gcc-patches at gcc dot gnu dot org>
- Date: Mon, 20 Nov 2017 21:10:04 +0000
- Subject: Re: [PATCH 1/3] [ARM] Add bus_width_bits to tune_params
- Authentication-results: sourceware.org; auth=none
- References: <1505205277-26276-1-git-send-email-charles.baylis@linaro.org> <1505205277-26276-2-git-send-email-charles.baylis@linaro.org> <59B8F42A.3050906@foss.arm.com> <CADnVucD+h2+wGJJuVm8-mzC8-hnCYfZc9yeGLU4rXmgVkvUH4w@mail.gmail.com> <59BC0755.9050205@foss.arm.com>
On 15 September 2017 at 18:01, Kyrill Tkachov
<kyrylo.tkachov@foss.arm.com> wrote:
> From what I can tell Ramana and Richard preferred to encode this attribute
> as
> a tuning struct property rather than an inline conditional based on
> arm_arch7.
> I agree that if we want to use that information, it should be encoded this
> way.
> What I'm not convinced about is whether we do want this parameter in the
> first place.
>
> The cost tables already encode information about the costs of different
> sized loads/stores.
> In patch 2, for example, you add the cost for extra_cost->ldst.load which is
> nominally just
> the cost of a normal 32-bit ldr. But we also have costs for ldst.ldrd which
> is the 64-bit two-register load
> which should reflect any extra cost due to a narrower bus in it. We also
> have costs for ldst.loadf (for 32-bit
> VFP loads) and ldst.loadd (for 64-bit VFP D-register loads). So I think we
> should use those cost fields
> depending on the mode class and size instead of using ldst.load
> unconditionally and adding a new bus_size parameter.
>
> So I think the way forward is to drop this patch and modify patch 2/3 to use
> the extra_cost->ldst fields as described above.
>
> Sorry for the back-and-forth. I think this is the best approach because it
> uses the existing fields more naturally and
> doesn't add new parameters that partly duplicate the information encoded in
> the existing fields.
> Ramana, Richard: if you prefer the bus_width approach I won't block it, but
> could you clarify your preference?
> If we do end up adding the bus_width parameter then this patch and patch 2/3
> look ok.
> Thanks,
> Kyrill
>
> P.S. I'm going on a 4-week holiday from today, so I won't be able to do any
> further review in that timeframe.
> As I said, if we go with the bus_size approach then these patches are ok. If
> we go with my suggestion, this would
> be dropped and patch 2 would be extended to select the appropriate
> extra_cost->ldst field depending on mode.
OK, I agree with dropping this patch. I have posted an updated patch 2
which does not require it.