This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH 4/6] Port prefetch configuration from aarch32 to aarch64


> On Jun 8, 2017, at 6:13 PM, Richard Earnshaw (lists) <Richard.Earnshaw@arm.com> wrote:
> 
> On 08/06/17 14:47, James Greenhalgh wrote:
>> On Mon, Jan 30, 2017 at 08:35:00AM -0800, Andrew Pinski wrote:
>>> On Mon, Jan 30, 2017 at 3:48 AM, Maxim Kuvyrkov
>>> <maxim.kuvyrkov@linaro.org> wrote:
>>>> This patch port prefetch configuration from aarch32 backend to aarch64.  There is no code-generation change from this patch.
>>>> 
>>>> This patch also happens to address Kyrill's comment on Andrew's prefetching patch at https://gcc.gnu.org/ml/gcc-patches/2017-01/msg02133.html .
>>>> 
>>>> This patch also fixes a minor bug in aarch64_override_options_internal(), which used "selected_cpu->tune" instead of "aarch64_tune_params".
>>> 
>>> I am not a fan of the macro at all.
>> 
>> I'm with Andrew for this. The precedent in the AArch64 port is for
>> explicitly spelling this out, as we do with the branch costs, approx_modes,
>> vector costs etc.
>> 
>> I'd rather we went that route than the macro you're using. I don't have
>> any objections to the rest of your patch.
>> 
>> Thanks,
>> James
>> 
> 
> I disagree with having to write all these things out, but I do agree
> that we should be self-consistent within the port.

I'm re-writing the patch with approach that James suggested, and instead of AARCH64_PREFETCH_NOT_BENEFICIAL there will be "&generic_prefetch_tune", so not much copy-paste of parameters.

> 
> The purpose of introducing the macros in the ARM port was to avoid the
> common problem of merging adding new parameters with adding new ports.
> C initializers for these tables assign values sequentially  with
> zero-padding at the end, so failing to merge such changes carefully
> leads to real bugs in the compiler (even if just performance bugs).
> 
> I notice that the last entry in the current tune params table is an int,
> rather than something that is type-checked (like the penultimate entry -
> an enum).  Having the final entry be type checked at least ensures that
> the right number of elements exist, even if the order is not strictly
> checked.

If you prefer, I can move "&generic_prefetch_tune" to the bottom of "struct tune_params", thus enabling type checking on struct pointer.

--
Maxim Kuvyrkov
www.linaro.org




Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]