On Fri, Jun 09, 2017 at 10:30:02AM +0300, Maxim Kuvyrkov wrote: > > On Jun 8, 2017, at 6:13 PM, Richard Earnshaw (lists) > > <richard.earns...@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.kuvyr...@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.
For the zero-cost/small-benefit tradefoff it gives, I'd say "why not". With that change in place and the obvious rebase needed for patch 6/6 in place, both this (4/6) and patch 6/6 are OK. Cheers, James
