[PATCH] D110258: [AArch64] Always add -tune-cpu argument to -cc1 driver

[David Sherwood via Phabricator via llvm-commits]

david-arm added a comment.

In D110258#3055488 <https://reviews.llvm.org/D110258#3055488>, @dmgreen wrote:

> If D111551 <https://reviews.llvm.org/D111551> was folded into this patch, would it be possible to add tests for -tune-cpu enabling/disabling features at the correct times?

Similar to the comment I left on D110259 <https://reviews.llvm.org/D110259>, I don't want D111551 <https://reviews.llvm.org/D111551> to hold up the cost model changes, which are critical. I'd prefer them to be independent. I expect D111551 <https://reviews.llvm.org/D111551> to take longer to get approval, and even once approved/merged if for any reason D111551 <https://reviews.llvm.org/D111551> causes issues after merging we only have to revert that one change to AArch64.td.



================
Comment at: clang/test/Driver/aarch64-mtune.c:3
+
+// There shouldn't be a default -mtune.
+// RUN: %clang -target aarch64-unknown-unknown -c -### %s 2>&1 \
----------------
dmgreen wrote:
> Why do we not want to add a default tune-cpu?
This was in response to an earlier review comment by @paulwalker-arm asking what benefit "-mtune=generic" provded and about restricting the patch to only add `tune-cpu` when the user has explicitly specified one.


================
Comment at: clang/test/Driver/aarch64-mtune.c:34
+
+// RUN: %clang -target aarch64-unknown-unknown -c -### %s -mcpu=thunderx 2>&1 \
+// RUN:   | FileCheck %s -check-prefix=mcputhunderx
----------------
dmgreen wrote:
> My understanding is that -mcpu=cpu is the same as -march=something + -mtune=cpu. Why would this case not add a -tune-cpu too? Is it because that gets handled in llvm?
I thought this was pretty standard behaviour? We're already adding -target-cpu, which implies the arch + tuning, so isn't adding -tune-cpu redundant? Not sure what value "-target-cpu=thunderx -tune-cpu=thunderx" adds really.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D110258/new/

https://reviews.llvm.org/D110258