This is an archive of the discontinued LLVM Phabricator instance.

[clang][Driver] Handle SPARC -mcpu=native etc.
ClosedPublic

Authored by ro on Jul 21 2022, 7:30 AM.

Details

Summary

To make use of SPARC support in getHostCPUName as implemented by D130272, this patch uses it to handle -mcpu=native and -mtune=native. To match GCC, this patch rejects -march instead of silently treating it as a no-op.

Tested on sparcv9-sun-solaris2.11 and checking that those options are passed on as -target-cpu resp. -tune-cpu as expected.

Diff Detail

Event Timeline

ro created this revision.Jul 21 2022, 7:30 AM
ro requested review of this revision.Jul 21 2022, 7:30 AM

I notice that in gcc, -march/-mtune/-mcpu are handled in gcc/config/* and every port may have somewhat different behaviors. E.g. x86 and aarch64 are different (and I suspect x86 has the weird behavior).

Have you checked that whether this matches GCC? We need some clang/test/Driver tests for "-cc1"{{.*}} "-target-cpu" "..."

MaskRay added inline comments.Jul 21 2022, 7:54 PM
clang/lib/Driver/ToolChains/Clang.cpp
2225

delete blank line after a variable declaration

2226
std::string TuneCPU(Name == "native" ? ... : ...)
if (!TuneCPU.empty()) {
  ...
ro added a comment.Jul 22 2022, 1:41 AM

I notice that in gcc, -march/-mtune/-mcpu are handled in gcc/config/* and every port may have somewhat different behaviors. E.g. x86 and aarch64 are different (and I suspect x86 has the weird behavior).

Right: e.g. gcc on SPARC doesn't support -march at all. I've made no changes to clang here, i.e. the option is silently ignored. I believe it's important to keep that compatibility so clang command lines can be passed to gcc or vice versa.

Have you checked that whether this matches GCC? We need some clang/test/Driver tests for "-cc1"{{.*}} "-target-cpu" "..."

Not in every detail, especially since the gcc driver just passes -mcpu etc. on to cc1 which handles them itself, so it's not really easy to see what happens.

I'll add some tests, though.

clang/lib/Driver/ToolChains/Clang.cpp
2226

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.

MaskRay accepted this revision.Jul 22 2022, 2:21 PM
MaskRay added inline comments.
clang/lib/Driver/ToolChains/Arch/Sparc.cpp
121

delete blank line

135

The common style omits else here

clang/lib/Driver/ToolChains/Clang.cpp
2226

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.

This revision is now accepted and ready to land.Jul 22 2022, 2:21 PM
ro updated this revision to Diff 448290.Jul 28 2022, 4:13 AM
ro marked 3 inline comments as done.
ro edited the summary of this revision. (Show Details)
  • Incorporate review comments.
  • Add testcases.
  • Reject -march, matching GCC.
This revision was landed with ongoing or failed builds.Jul 29 2022, 12:27 AM
This revision was automatically updated to reflect the committed changes.