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

[david-arm]

Summary

This patch ensures that we always tune for a given CPU on AArch64
targets when the user specifies the "-mtune=xyz" flag. In the
AArch64Subtarget if the tune flag is unset we use the CPU value
instead.

Tests added here:

clang/test/Driver/aarch64-mtune.c

[dmgreen]

My understanding is that -mtune doesn't work sensibly for Arm backends. The tuning features and architecture features are not separated out at the subtarget level.

Is the idea to teach people to start using it? That sounds dangerous without fixing the issues with it first. Or is this to make sure it's set to something?

(Fixing it properly would be a really good thing.)

[david-arm]

Hi @dmgreen, this is specifically being introduced for SVE targets to help make informed cost model decisions regarding the value of vscale - see D110259. We thought that using the "tune-cpu" attribute might be a good way of doing this.

[dmgreen]

In D110258#3015484, @david-arm wrote:

Hi @dmgreen, this is specifically being introduced for SVE targets to help make informed cost model decisions regarding the value of vscale - see D110259. We thought that using the "tune-cpu" attribute might be a good way of doing this.

Yeah. I added comments there. It would feel strange to me to have a single part of the tuning features for a cpu based on a tune-cpu, where everything else uses the target-cpu. We should try and make -mtune work better, but we should aim to do that consistently in a way that doesn't just make thing _more_ confusing!

[paulwalker-arm]

clang/lib/Driver/ToolChains/Clang.cpp

1850

What benefit does -tune-cpu generic provide?

I'm wondering if the patch can be restricted to only add -tune-cpu when a -mtune= is specified with a real name or a detected name for when "native" is specified.

1857–1860

See me comment above.

[david-arm]

• Updated the patch to use mtune properly and set the scheduling model accordingly

[dmgreen]

Sounds great. Glad to see us taking this route.

Unfortunately I think we do need to split the subtargetfeatures up into arch flags and tune flags. Same for the details in AArch64Subtarget::initializeProperties. It is hopefully a fairly mechanical process, but they are an important part of tuning and without them -mtune is only a half-working option.

Are you happy to give that a go too?

llvm/lib/Target/AArch64/AArch64Subtarget.h

305

Needs reformatting.

[david-arm]

In D110258#3023818, @dmgreen wrote:

Sounds great. Glad to see us taking this route.

Unfortunately I think we do need to split the subtargetfeatures up into arch flags and tune flags. Same for the details in AArch64Subtarget::initializeProperties. It is hopefully a fairly mechanical process, but they are an important part of tuning and without them -mtune is only a half-working option.

Are you happy to give that a go too?

Hi @dmgreen, sure I can try. The only problem is that I don't really understand what to do here. I used the X86Subtarget as a guidance and the TuneCPU flag only seems to be used for scheduling and nothing else. It's not obvious to me how the TuneCPU flag decides the features as I thought it was purely for scheduling?

[dmgreen]

I think ProcessorModel is

class ProcessorModel<string n, SchedMachineModel m, list<SubtargetFeature> f,
                     list<SubtargetFeature> tunef = []>

So we are always currently passing tunef = [] from AArch64 and passing all features through f. They need to be split out and then hopefully the call to ParseSubtargetFeatures will use CPU and TuneCPU appropriately.

[dmgreen]

I think these are the features I would class as tuning:

FeatureExperimentalZeroingPseudos
FeatureZCRegMove
FeatureZCZeroingGP
FeatureNoZCZeroingFP
FeatureZCZeroing
FeatureZCZeroingFPWorkaround
FeatureStrictAlign
FeatureBalanceFPOps
FeaturePredictableSelectIsExpensive
FeatureCustomCheapAsMoveHandling
FeatureExynosCheapAsMoveHandling
FeaturePostRAScheduler
FeatureSlowMisaligned128Store
FeatureSlowPaired128
FeatureSlowSTRQro
FeatureAlternateSExtLoadCVTF32Pattern
FeatureArithmeticBccFusion
FeatureArithmeticCbzFusion
FeatureCmpBccFusion
FeatureFuseAddress
FeatureFuseAES
FeatureFuseArithmeticLogic
FeatureFuseCCSelect
FeatureFuseCryptoEOR
FeatureFuseLiterals
FeatureDisableLatencySchedHeuristic
FeatureForce32BitJumpTables
FeatureUseRSqrt
FeatureLSLFast
FeatureAggressiveFMA

The ProcA55 type features set ARMProcFamily, which seems to be used for tuning everywhere they are used, so would probably class as tuning features too.

[david-arm]

Hi @dmgreen, would you be happy for me to do the splitting-out of arch and tuning features in a separate follow-on patch? I think it's a good idea and I don't object to doing it, but I'm not sure that it really needs to hold up this initial patch? I personally think it makes sense to live in a separate patch because it seems riskier in terms of possible effects on performance. As far as I understand it, this isn't a functional requirement, but more for completeness.

[dmgreen]

In D110258#3024418, @david-arm wrote:

Hi @dmgreen, would you be happy for me to do the splitting-out of arch and tuning features in a separate follow-on patch? I think it's a good idea and I don't object to doing it, but I'm not sure that it really needs to hold up this initial patch? I personally think it makes sense to live in a separate patch because it seems riskier in terms of possible effects on performance. As far as I understand it, this isn't a functional requirement, but more for completeness.

I think that the ARMProcFamily subtarget features are really tuning features, which would mean that the AArch64Subtarget::initializeProperties would end up being based on tuning features and the code in D110259 would simplify as a result, not needing to pass TuneCPU around and check for specific cpus. We could change how that works code later, but I am a little worried about it being half finished, having tune-cpu not really working as it should but being required for some features. I would be fine as a separate patch so long as we are pretty confident that -mtune was going to work correctly before we start using it.

[david-arm]

• Fixed formatting issues.

[david-arm]

Gentle ping!

[dmgreen]

If D111551 was folded into this patch, would it be possible to add tests for -tune-cpu enabling/disabling features at the correct times?

clang/test/Driver/aarch64-mtune.c

4

Why do we not want to add a default tune-cpu?

35

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?

[david-arm]

In D110258#3055488, @dmgreen wrote:

If 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, I don't want D111551 to hold up the cost model changes, which are critical. I'd prefer them to be independent. I expect D111551 to take longer to get approval, and even once approved/merged if for any reason D111551 causes issues after merging we only have to revert that one change to AArch64.td.

clang/test/Driver/aarch64-mtune.c

4

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.

35

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.

[dmgreen]

In D110258#3057380, @david-arm wrote:

In D110258#3055488, @dmgreen wrote:

If 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, I don't want D111551 to hold up the cost model changes, which are critical.

They are not. The changes in the cost model in 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.

clang/test/Driver/aarch64-mtune.c

35

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.

[dmgreen]

Now that we have the tests on the D111551 side, I think this patch LGTM.

Do you think it's worth adding something to the release notes? "-mtune now actually works for AArch64". But, umm, perhaps better written than that.

[david-arm]

• Added something to the ReleaseNotes file.

[dmgreen]

There are clang release notes that could also do with a line or two, after all the -mtune flag being a clang flag. The X86 notes for reference were added in https://releases.llvm.org/12.0.0/docs/ReleaseNotes.html and https://releases.llvm.org/12.0.0/tools/clang/docs/ReleaseNotes.html, from the look of it. I like "no longer ignored", but don't really have a strong opinion on the exact wording.

[david-arm]

• Added release notes for both clang and llvm.

[sdesmalen]

clang/docs/ReleaseNotes.rst

185–188 ↗

(On Diff #380649)

nit: compiling with "-mcpu=generic -mtune=cortex-a57" will not enable any Cortex-a57 specific architecture features, but will enable certain optimizations specific to Cortex-a57 CPUs and enable the use of a more accurate scheduling model.

clang/test/Driver/aarch64-mtune.c

6

nit: Did you make these prefixes lower-case on purpose?

llvm/docs/ReleaseNotes.rst

80 ↗

(On Diff #380649)

nit: s/we tune/it tunes/

[david-arm]

clang/test/Driver/aarch64-mtune.c

6

Sort of, in that I copied this test from x86-mtune.c, which has the same CHECK prefixes. :) I can capitalise them.

[dmgreen]

Thanks. I'm happy as soon as everyone else is happy with the wording.

[sdesmalen]

Thanks @david-arm, LGTM

clang/lib/Driver/ToolChains/Clang.cpp

if (Arg *A = Args.getLastArg(options::OPT_msve_vector_bits_EQ)) {
     // Silently drop requests for vector-length agnostic code as it's implied.
     else if (!Val.equals("scalable"))
       // Handle the unsupported values passed to msve-vector-bits.
       D.Diag(diag::err_drv_unsupported_option_argument)
           << A->getOption().getName() << Val;
   }
 
   AddAAPCSVolatileBitfieldArgs(Args, CmdArgs);
+
+  if (const Arg *A = Args.getLastArg(clang::driver::options::OPT_mtune_EQ)) {
+    StringRef Name = A->getValue();
+
+    std::string TuneCPU;
+    if (Name == "native")
+      TuneCPU = std::string(llvm::sys::getHostCPUName());
+    else
+      TuneCPU = std::string(Name);
+
+    if (!TuneCPU.empty()) {
+      CmdArgs.push_back("-tune-cpu");
+      CmdArgs.push_back(Args.MakeArgString(TuneCPU));
+    }
+  }
 }
 
 void Clang::AddMIPSTargetArgs(const ArgList &Args,
                               ArgStringList &CmdArgs) const {
   const Driver &D = getToolChain().getDriver();
   StringRef CPUName;
   StringRef ABIName;
   const llvm::Triple &Triple = getToolChain().getTriple();
   mips::getMipsCPUAndABI(Args, Triple, CPUName, ABIName);
 
   CmdArgs.push_back("-target-abi");
   CmdArgs.push_back(ABIName.data());
 
   mips::FloatABI ABI = mips::getMipsFloatABI(D, Args, Triple);

clang/test/Driver/aarch64-mtune.c

+// Ensure we support the -mtune flag.
+
+// There shouldn't be a default -mtune.
+// RUN: %clang -target aarch64-unknown-unknown -c -### %s 2>&1 \
+// RUN:   | FileCheck %s -check-prefix=notune
+// notune-NOT: "-tune-cpu" "generic"
+
+// RUN: %clang -target aarch64-unknown-unknown -c -### %s -mtune=generic 2>&1 \
+// RUN:   | FileCheck %s -check-prefix=generic
+// generic: "-tune-cpu" "generic"
+
+// RUN: %clang -target aarch64-unknown-unknown -c -### %s -mtune=neoverse-n1 2>&1 \
+// RUN:   | FileCheck %s -check-prefix=neoverse-n1
+// neoverse-n1: "-tune-cpu" "neoverse-n1"
+
+// RUN: %clang -target aarch64-unknown-unknown -c -### %s -mtune=thunderx2t99 2>&1 \
+// RUN:   | FileCheck %s -check-prefix=thunderx2t99
+// thunderx2t99: "-tune-cpu" "thunderx2t99"
+
+// Check interaction between march and mtune.
+
+// RUN: %clang -target aarch64-unknown-unknown -c -### %s -march=armv8-a 2>&1 \
+// RUN:   | FileCheck %s -check-prefix=marcharmv8a
+// marcharmv8a: "-target-cpu" "generic"
+// marcharmv8a-NOT: "-tune-cpu" "generic"
+
+// RUN: %clang -target aarch64-unknown-unknown -c -### %s -march=armv8-a -mtune=cortex-a75 2>&1 \
+// RUN:   | FileCheck %s -check-prefix=marcharmv8a-a75
+// marcharmv8a-a75: "-target-cpu" "generic"
+// marcharmv8a-a75: "-tune-cpu" "cortex-a75"
+
+// Check interaction between mcpu and mtune.
+
+// RUN: %clang -target aarch64-unknown-unknown -c -### %s -mcpu=thunderx 2>&1 \
+// RUN:   | FileCheck %s -check-prefix=mcputhunderx
+// mcputhunderx: "-target-cpu" "thunderx"
+// mcputhunderx-NOT: "-tune-cpu"
+
+// RUN: %clang -target aarch64-unknown-unknown -c -### %s -mcpu=cortex-a75 -mtune=cortex-a57 2>&1 \
+// RUN:   | FileCheck %s -check-prefix=mcpua75-mtunea57
+// mcpua75-mtunea57: "-target-cpu" "cortex-a75"
+// mcpua75-mtunea57: "-tune-cpu" "cortex-a57"

llvm/lib/Target/AArch64/AArch64Subtarget.h

protected:
   std::unique_ptr<LegalizerInfo> Legalizer;
   std::unique_ptr<RegisterBankInfo> RegBankInfo;
 
 private:
   /// initializeSubtargetDependencies - Initializes using CPUString and the
   /// passed in feature string so that we can use initializer lists for
   /// subtarget initialization.
   AArch64Subtarget &initializeSubtargetDependencies(StringRef FS,
-                                                    StringRef CPUString);
+                                                    StringRef CPUString,
+                                                    StringRef TuneCPUString);
 
   /// Initialize properties based on the selected processor family.
   void initializeProperties();
 
 public:
   /// This constructor initializes the data members to match that
   /// of the specified triple.
-  AArch64Subtarget(const Triple &TT, const std::string &CPU,
+  AArch64Subtarget(const Triple &TT, const std::string &CPU, const std::string &TuneCPU,
                    const std::string &FS, const TargetMachine &TM,
                    bool LittleEndian,
                    unsigned MinSVEVectorSizeInBitsOverride = 0,
                    unsigned MaxSVEVectorSizeInBitsOverride = 0);
 
   const AArch64SelectionDAGInfo *getSelectionDAGInfo() const override {
     return &TSInfo;
   }

llvm/lib/Target/AArch64/AArch64Subtarget.cpp

 static cl::opt<bool>
     UseNonLazyBind("aarch64-enable-nonlazybind",
                    cl::desc("Call nonlazybind functions via direct GOT load"),
                    cl::init(false), cl::Hidden);
 
 static cl::opt<bool> UseAA("aarch64-use-aa", cl::init(true),
                            cl::desc("Enable the use of AA during codegen."));
 
 AArch64Subtarget &
 AArch64Subtarget::initializeSubtargetDependencies(StringRef FS,
-                                                  StringRef CPUString) {
+                                                  StringRef CPUString,
+                                                  StringRef TuneCPUString) {
   // Determine default and user-specified characteristics
 
   if (CPUString.empty())
     CPUString = "generic";
 
-  ParseSubtargetFeatures(CPUString, /*TuneCPU*/ CPUString, FS);
+  if (TuneCPUString.empty())
+    TuneCPUString = CPUString;
+
+  ParseSubtargetFeatures(CPUString, TuneCPUString, FS);
   initializeProperties();
 
   return *this;
 }
 
 void AArch64Subtarget::initializeProperties() {
   // Initialize CPU specific properties. We should add a tablegen feature for
   // this in the future so we can specify it together with the subtarget
case ThunderX3T110:
     MinPrefetchStride = 1024;
     MaxPrefetchIterationsAhead = 4;
     // FIXME: remove this to enable 64-bit SLP if performance looks good.
     MinVectorRegisterBitWidth = 128;
     break;
   }
 }
 
-AArch64Subtarget::AArch64Subtarget(const Triple &TT, const std::string &CPU,
+AArch64Subtarget::AArch64Subtarget(const Triple &TT, const std::string &CPU, const std::string &TuneCPU,
                                    const std::string &FS,
                                    const TargetMachine &TM, bool LittleEndian,
                                    unsigned MinSVEVectorSizeInBitsOverride,
                                    unsigned MaxSVEVectorSizeInBitsOverride)
-    : AArch64GenSubtargetInfo(TT, CPU, /*TuneCPU*/ CPU, FS),
+    : AArch64GenSubtargetInfo(TT, CPU, TuneCPU, FS),
       ReserveXRegister(AArch64::GPR64commonRegClass.getNumRegs()),
       CustomCallSavedXRegs(AArch64::GPR64commonRegClass.getNumRegs()),
       IsLittle(LittleEndian),
       MinSVEVectorSizeInBits(MinSVEVectorSizeInBitsOverride),
       MaxSVEVectorSizeInBits(MaxSVEVectorSizeInBitsOverride), TargetTriple(TT),
-      FrameLowering(), InstrInfo(initializeSubtargetDependencies(FS, CPU)),
+      FrameLowering(), InstrInfo(initializeSubtargetDependencies(FS, CPU, TuneCPU)),
       TSInfo(), TLInfo(TM, *this) {
   if (AArch64::isX18ReservedByDefault(TT))
     ReserveXRegister.set(18);
 
   CallLoweringInfo.reset(new AArch64CallLowering(*getTargetLowering()));
   InlineAsmLoweringInfo.reset(new InlineAsmLowering(getTargetLowering()));
   Legalizer.reset(new AArch64LegalizerInfo(*this));
 

llvm/lib/Target/AArch64/AArch64TargetMachine.cpp

AArch64TargetMachine::AArch64TargetMachine(const Target &T, const Triple &TT,
   setSupportsDebugEntryValues(true);
 }
 
 AArch64TargetMachine::~AArch64TargetMachine() = default;
 
 const AArch64Subtarget *
 AArch64TargetMachine::getSubtargetImpl(const Function &F) const {
   Attribute CPUAttr = F.getFnAttribute("target-cpu");
+  Attribute TuneAttr = F.getFnAttribute("tune-cpu");
   Attribute FSAttr = F.getFnAttribute("target-features");
 
   std::string CPU =
       CPUAttr.isValid() ? CPUAttr.getValueAsString().str() : TargetCPU;
+  std::string TuneCPU =
+      TuneAttr.isValid() ? TuneAttr.getValueAsString().str() : CPU;
   std::string FS =
       FSAttr.isValid() ? FSAttr.getValueAsString().str() : TargetFS;
 
   SmallString<512> Key;
 
   unsigned MinSVEVectorSize = 0;
   unsigned MaxSVEVectorSize = 0;
   Attribute VScaleRangeAttr = F.getFnAttribute(Attribute::VScaleRange);
MaxSVEVectorSize =
         (std::max(MinSVEVectorSize, MaxSVEVectorSize) / 128) * 128;
   }
 
   Key += "SVEMin";
   Key += std::to_string(MinSVEVectorSize);
   Key += "SVEMax";
   Key += std::to_string(MaxSVEVectorSize);
   Key += CPU;
+  Key += TuneCPU;
   Key += FS;
 
   auto &I = SubtargetMap[Key];
   if (!I) {
     // This needs to be done before we create a new subtarget since any
     // creation will depend on the TM and the code generation flags on the
     // function that reside in TargetOptions.
     resetTargetOptions(F);
-    I = std::make_unique<AArch64Subtarget>(TargetTriple, CPU, FS, *this,
+    I = std::make_unique<AArch64Subtarget>(TargetTriple, CPU, TuneCPU, FS, *this,
                                            isLittle, MinSVEVectorSize,
                                            MaxSVEVectorSize);
   }
   return I.get();
 }
 
 void AArch64leTargetMachine::anchor() { }
 

llvm/unittests/Target/AArch64/InstSizes.cpp

std::unique_ptr<LLVMTargetMachine> createTargetMachine() {
 
   return std::unique_ptr<LLVMTargetMachine>(static_cast<LLVMTargetMachine*>(
       TheTarget->createTargetMachine(TT, CPU, FS, TargetOptions(), None, None,
                                      CodeGenOpt::Default)));
 }
 
 std::unique_ptr<AArch64InstrInfo> createInstrInfo(TargetMachine *TM) {
   AArch64Subtarget ST(TM->getTargetTriple(), std::string(TM->getTargetCPU()),
+                      std::string(TM->getTargetCPU()),
                       std::string(TM->getTargetFeatureString()), *TM,
                       /* isLittle */ false);
   return std::make_unique<AArch64InstrInfo>(ST);
 }
 
 /// The \p InputIRSnippet is only needed for things that can't be expressed in
 /// the \p InputMIRSnippet (global variables etc)
 /// TODO: Some of this might be useful for other architectures as well - extract

llvm/unittests/Target/AArch64/MatrixRegisterAliasing.cpp

std::unique_ptr<LLVMTargetMachine> createTargetMachine() {
 
   return std::unique_ptr<LLVMTargetMachine>(
       static_cast<LLVMTargetMachine *>(TheTarget->createTargetMachine(
           TT, CPU, FS, TargetOptions(), None, None, CodeGenOpt::Default)));
 }
 
 std::unique_ptr<AArch64InstrInfo> createInstrInfo(TargetMachine *TM) {
   AArch64Subtarget ST(TM->getTargetTriple(), std::string(TM->getTargetCPU()),
+                      std::string(TM->getTargetCPU()),
                       std::string(TM->getTargetFeatureString()), *TM,
                       /* isLittle */ false);
   return std::make_unique<AArch64InstrInfo>(ST);
 }
 
 TEST(MatrixRegisterAliasing, Aliasing) {
   std::unique_ptr<LLVMTargetMachine> TM = createTargetMachine();
   ASSERT_TRUE(TM);