[PATCH] D130273: [clang][Driver] Handle SPARC -mcpu=native etc.

[Fangrui Song via Phabricator via cfe-commits]

MaskRay accepted this revision.
MaskRay added inline comments.
This revision is now accepted and ready to land.


================
Comment at: clang/lib/Driver/ToolChains/Arch/Sparc.cpp:121
+    StringRef CPUName = A->getValue();
+
+    if (CPUName == "native") {
----------------
delete blank line


================
Comment at: clang/lib/Driver/ToolChains/Arch/Sparc.cpp:135
+    return "v9";
+  else
+    return "";
----------------
The common style omits `else` here


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:2226
+
+    std::string TuneCPU;
+    if (Name == "native")
----------------
ro wrote:
> MaskRay wrote:
> > ```
> > std::string TuneCPU(Name == "native" ? ... : ...)
> > if (!TuneCPU.empty()) {
> >   ...
> > ```
> I'm not sure about this: I tried that variant, but I don't really think it's clearer than what I have now:
> ```
>     std::string TuneCPU(Name == "native"
>                             ? std::string(llvm::sys::getHostCPUName()
>                             : std::string(Name)));
> ```
> My code was taken from `AddSystemZTargetArgs` directly below and it would seem a bit weird if they differ in style.
OK, but I think AddSystemZTargetArgs somewhat deviates from common styles.

Since `llvm::sys::getHostCPUName()` cannot be empty, and `-mtune=` (empty value) should be an error (but only aarch64 seems to emit an error), I'd omit the `if (!TuneCPU.empty()) ` test. For aarch64, I made such a simplification: 475e526d85003404ba521e15f8acef1b439fb910

I don't mind whether sparc emits an error for `-mtune=` or not.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130273