dmgreen added a comment. In D110258#3057380 <https://reviews.llvm.org/D110258#3057380>, @david-arm wrote:
> 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. They are not. The changes in the cost model in D110259 <https://reviews.llvm.org/D110259> are independent of whether -mcpu or -mtune work a particular way and could probably be committed now if it was rebased away from the mtune patches :) > I'd prefer them to be independent. That's fine. They seem like two patches doing two halves of one change to me, but having them separate sounds perfectly fine so long as we have the tests we need. I see tests that the clang -mcpu and -mtune set llvm flags as expected, but not that those target-cpu and tune-cpu options then set the correct features at the correct times. ================ 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 ---------------- david-arm wrote: > 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. It could work either way with adding both attributes or letting the target-cpu act as both if it is the only one present. So long as we are happy with it working this way it sounds good to me. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D110258/new/ https://reviews.llvm.org/D110258 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
