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. 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. My 2p. R.
