Let llc and opt override "-target-cpu" and "-target-features" using command line options "-mcpu" and "-mattr"

[ahatanak]

Summary

llc's options -mcpu and -mattr have no effect if the functions in the IR already have function attributes -target-cpu and -target-features. This is bad for llvm developers as they don't have a convenient way to quickly test different target-cpu and target-features via command line options.

This patch attempts to fix this bug by letting llc and opt rewrite the attributes in the IR. An alternate way would be to pass the cpu and feature strings in TargetOptions but I chose the approach I took in the patch to avoid making changes to TargetOptions and XXXTargetMachine. Also, factored out helper function getCPUFeaturesStr which both opt and llc call.

[echristo]

I agree, no TargetOptions for this. I don't necessarily agree with rewriting, but it seems to be a small enough change and will work for what the overrides are meant to be for. Some discipline in reviewing testcases to make sure that a lot of overrides aren't there for incoming testcases is going to be necessary for clean tests. One inline comment which basically sums up to "why not two functions"? As long as you have a reasonable answer go ahead.

Thanks!

-eric

include/llvm/CodeGen/CommandFlags.h

268

Why a pair?

[ahatanak]

In D9537#167157, @echristo wrote:

I agree, no TargetOptions for this. I don't necessarily agree with rewriting, but it seems to be a small enough change and will work for what the overrides are meant to be for. Some discipline in reviewing testcases to make sure that a lot of overrides aren't there for incoming testcases is going to be necessary for clean tests. One inline comment which basically sums up to "why not two functions"? As long as you have a reasonable answer go ahead.

I'm not sure if I understood your comment about testcases. Do you mean people shouldn't duplicate functions in test cases just to test different target-cpu and target-feature strings but instead should use -mcpu and -mattrs (which I think makes sense)? Should the one inline comment go into the test cases I added?

include/llvm/CodeGen/CommandFlags.h

268

Is it better to split this into two functions getCPUStr and getFeaturesStr? Or you meant the two strings (cpu and features strings) should be passed by reference?

include/llvm/CodeGen/CommandFlags.h

 // command line tools. The tools "llc" and "opt" both use this file to prevent
 // flag duplication.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CODEGEN_COMMANDFLAGS_H
 #define LLVM_CODEGEN_COMMANDFLAGS_H
 
+#include "llvm/IR/Function.h"
+#include "llvm/IR/LLVMContext.h"
+#include "llvm/IR/Module.h"
 #include "llvm/MC/MCTargetOptionsCommandFlags.h"
+#include "llvm//MC/SubtargetFeature.h"
 #include "llvm/Support/CodeGen.h"
 #include "llvm/Support/CommandLine.h"
+#include "llvm/Support/Host.h"
 #include "llvm/Target/TargetMachine.h"
 #include "llvm/Target/TargetOptions.h"
 #include <string>
 using namespace llvm;
 
 cl::opt<std::string>
 MArch("march", cl::desc("Architecture to generate code for (see --version)"));
 
static inline TargetOptions InitTargetOptionsFromCodeGenFlags() {
   Options.MCOptions = InitMCTargetOptionsFromFlags();
   Options.JTType = JTableType;
 
   Options.ThreadModel = TMModel;
 
   return Options;
 }
 
+static inline std::pair<std::string, std::string> getCPUFeaturesStr() {
+  std::string CPUStr;
+
+  // If user asked for the 'native' CPU, autodetect here. If autodection fails,
+  // this will set the CPU to an empty string which tells the target to
+  // pick a basic default.
+  if (MCPU == "native")
+    CPUStr = sys::getHostCPUName();
+  else
+    CPUStr = MCPU;
+
+  SubtargetFeatures Features;
+
+  // If user asked for the 'native' CPU, we need to autodetect features.
+  // This is necessary for x86 where the CPU might not support all the
+  // features the autodetected CPU name lists in the target. For example,
+  if (MCPU == "native") {
+    StringMap<bool> HostFeatures;
+    if (sys::getHostCPUFeatures(HostFeatures))
+      for (auto &F : HostFeatures)
+        Features.AddFeature(F.first(), F.second);
+  }
+
+  for (unsigned i = 0; i != MAttrs.size(); ++i)
+    Features.AddFeature(MAttrs[i]);
+
+  return std::make_pair(CPUStr, Features.getString());
+}
+
+static inline void overrideFunctionAttributes(StringRef CPU, StringRef Features,
+                                              Module &M) {
+  for (auto &F : M) {
+    if (!CPU.empty())
+      llvm::overrideFunctionAttribute("target-cpu", CPU, F);
+
+    if (!Features.empty())
+      llvm::overrideFunctionAttribute("target-features", Features, F);
+  }
+}
+
 #endif

include/llvm/IR/Function.h

ilist_traits<BasicBlock>::getSymTab(Function *F) {
   return F ? &F->getValueSymbolTable() : nullptr;
 }
 
 inline ValueSymbolTable *
 ilist_traits<Argument>::getSymTab(Function *F) {
   return F ? &F->getValueSymbolTable() : nullptr;
 }
 
+void overrideFunctionAttribute(StringRef Kind, StringRef Value, Function &F);
+
 } // End llvm namespace
 
 #endif

lib/IR/Function.cpp

if (PrologueData) {
     PDData |= (1<<2);
   } else {
     delete PDHolder;
     PDMap.erase(this);
     PDData &= ~(1<<2);
   }
   setValueSubclassData(PDData);
 }
+
+void llvm::overrideFunctionAttribute(StringRef Kind, StringRef Value,
+                                     Function &F) {
+  auto &Ctx = F.getContext();
+  AttributeSet Attrs = F.getAttributes(), AttrsToRemove;
+
+  AttrsToRemove =
+      AttrsToRemove.addAttribute(Ctx, AttributeSet::FunctionIndex, Kind);
+  Attrs = Attrs.removeAttributes(Ctx, AttributeSet::FunctionIndex,
+                                 AttrsToRemove);
+  Attrs = Attrs.addAttribute(Ctx, AttributeSet::FunctionIndex, Kind, Value);
+  F.setAttributes(Attrs);
+}

test/CodeGen/X86/llc-override-mcpu-mattr.ll

+; RUN: llc < %s -march x86-64 -mcpu=broadwell | FileCheck %s
+; RUN: llc < %s -march x86-64 -mattr=+avx2 | FileCheck %s
+
+; CHECK: vpsadbw %ymm{{[0-9]+}}, %ymm{{[0-9]+}}, %ymm{{[0-9]+}}
+
+define <4 x i64> @foo1(<4 x i64> %s1, <4 x i64> %s2) #0 {
+entry:
+  %0 = bitcast <4 x i64> %s1 to <32 x i8>
+  %1 = bitcast <4 x i64> %s2 to <32 x i8>
+  %2 = tail call <4 x i64> @llvm.x86.avx2.psad.bw(<32 x i8> %0, <32 x i8> %1)
+  ret <4 x i64> %2
+}
+
+declare <4 x i64> @llvm.x86.avx2.psad.bw(<32 x i8>, <32 x i8>)
+
+attributes #0 = { "target-cpu"="core2" "target-features"="+ssse3,+cx16,+sse4.2,+sse4.1,+sse,+sse2,+sse3,+avx,+popcnt" }

test/Other/opt-override-mcpu-mattr.ll

+; RUN: opt < %s -mtriple=x86_64-apple-darwin -mcpu=broadwell -mattr=+avx2 -S | FileCheck %s
+
+; CHECK: attributes #0 = { nounwind readnone ssp uwtable "target-cpu"="broadwell" "target-features"="+avx2" "use-soft-float"="false" }
+
+define i32 @foo1() #0 {
+entry:
+  ret i32 0
+}
+
+attributes #0 = { nounwind readnone ssp uwtable "target-cpu"="core2" "target-features"="+ssse3,+cx16,+sse,+sse2,+sse3" "use-soft-float"="false" }

test/Transforms/SLPVectorizer/X86/call.ll

entry:
   %call5 = tail call i64 @round(i64 %mul5) nounwind readnone
   store i64 %call, i64* %c, align 8
   %arrayidx5 = getelementptr inbounds i64, i64* %c, i64 1
   store i64 %call5, i64* %arrayidx5, align 8
   ret void
 }
 
 
-; CHECK: declare <2 x double> @llvm.sin.v2f64(<2 x double>) #0
-; CHECK: declare <2 x double> @llvm.cos.v2f64(<2 x double>) #0
-; CHECK: declare <2 x double> @llvm.pow.v2f64(<2 x double>, <2 x double>) #0
-; CHECK: declare <2 x double> @llvm.exp2.v2f64(<2 x double>) #0
+; CHECK: declare <2 x double> @llvm.sin.v2f64(<2 x double>) [[ATTR0:#[0-9]+]]
+; CHECK: declare <2 x double> @llvm.cos.v2f64(<2 x double>) [[ATTR0]]
+; CHECK: declare <2 x double> @llvm.pow.v2f64(<2 x double>, <2 x double>) [[ATTR0]]
+; CHECK: declare <2 x double> @llvm.exp2.v2f64(<2 x double>) [[ATTR0]]
 
-; CHECK: attributes #0 = { nounwind readnone }
+; CHECK: attributes [[ATTR0]] = { nounwind readnone }
 

tools/llc/llc.cpp

static int compileModule(char **argv, LLVMContext &Context) {
   std::string Error;
   const Target *TheTarget = TargetRegistry::lookupTarget(MArch, TheTriple,
                                                          Error);
   if (!TheTarget) {
     errs() << argv[0] << ": " << Error;
     return 1;
   }
 
-  // Package up features to be passed to target/subtarget
-  std::string FeaturesStr;
-  if (!MAttrs.empty() || MCPU == "native") {
-    SubtargetFeatures Features;
-
-    // If user asked for the 'native' CPU, we need to autodetect features.
-    // This is necessary for x86 where the CPU might not support all the
-    // features the autodetected CPU name lists in the target. For example,
-    // not all Sandybridge processors support AVX.
-    if (MCPU == "native") {
-      StringMap<bool> HostFeatures;
-      if (sys::getHostCPUFeatures(HostFeatures))
-        for (auto &F : HostFeatures)
-          Features.AddFeature(F.first(), F.second);
-    }
-
-    for (unsigned i = 0; i != MAttrs.size(); ++i)
-      Features.AddFeature(MAttrs[i]);
-    FeaturesStr = Features.getString();
-  }
-
-  // If user asked for the 'native' CPU, autodetect here. If autodection fails,
-  // this will set the CPU to an empty string which tells the target to
-  // pick a basic default.
-  if (MCPU == "native")
-    MCPU = sys::getHostCPUName();
+  std::pair<std::string, std::string> CPUFeatures = getCPUFeaturesStr();
 
   CodeGenOpt::Level OLvl = CodeGenOpt::Default;
   switch (OptLevel) {
   default:
     errs() << argv[0] << ": invalid optimization level.\n";
     return 1;
   case ' ': break;
   case '0': OLvl = CodeGenOpt::None; break;
   case '1': OLvl = CodeGenOpt::Less; break;
   case '2': OLvl = CodeGenOpt::Default; break;
   case '3': OLvl = CodeGenOpt::Aggressive; break;
   }
 
   TargetOptions Options = InitTargetOptionsFromCodeGenFlags();
   Options.DisableIntegratedAS = NoIntegratedAssembler;
   Options.MCOptions.ShowMCEncoding = ShowMCEncoding;
   Options.MCOptions.MCUseDwarfDirectory = EnableDwarfDirectory;
   Options.MCOptions.AsmVerbose = AsmVerbose;
 
   std::unique_ptr<TargetMachine> Target(
-      TheTarget->createTargetMachine(TheTriple.getTriple(), MCPU, FeaturesStr,
-                                     Options, RelocModel, CMModel, OLvl));
+      TheTarget->createTargetMachine(TheTriple.getTriple(), CPUFeatures.first,
+                                     CPUFeatures.second, Options, RelocModel,
+                                     CMModel, OLvl));
+
   assert(Target && "Could not allocate target machine!");
 
   // If we don't have a module then just exit now. We do this down
   // here since the CPU/Feature help is underneath the target machine
   // creation.
   if (SkipModule)
     return 0;
 
static int compileModule(char **argv, LLVMContext &Context) {
   if (DisableSimplifyLibCalls)
     TLII.disableAllFunctions();
   PM.add(new TargetLibraryInfoWrapperPass(TLII));
 
   // Add the target data from the target machine, if it exists, or the module.
   if (const DataLayout *DL = Target->getDataLayout())
     M->setDataLayout(*DL);
 
+  // Override function attributes.
+  overrideFunctionAttributes(CPUFeatures.first, CPUFeatures.second, *M);
+
   if (RelaxAll.getNumOccurrences() > 0 &&
       FileType != TargetMachine::CGFT_ObjectFile)
     errs() << argv[0]
              << ": warning: ignoring -mc-relax-all because filetype != obj";
 
   {
     raw_pwrite_stream *OS = &Out->os();
     std::unique_ptr<buffer_ostream> BOS;

tools/opt/opt.cpp

static CodeGenOpt::Level GetCodeGenOptLevel() {
   if (OptLevelO2)
     return CodeGenOpt::Default;
   if (OptLevelO3)
     return CodeGenOpt::Aggressive;
   return CodeGenOpt::None;
 }
 
 // Returns the TargetMachine instance or zero if no triple is provided.
-static TargetMachine* GetTargetMachine(Triple TheTriple) {
+static TargetMachine* GetTargetMachine(Triple TheTriple, StringRef CPUStr,
+                                       StringRef FeaturesStr) {
   std::string Error;
   const Target *TheTarget = TargetRegistry::lookupTarget(MArch, TheTriple,
                                                          Error);
   // Some modules don't specify a triple, and this is okay.
   if (!TheTarget) {
     return nullptr;
   }
 
-  // Package up features to be passed to target/subtarget
-  std::string FeaturesStr;
-  if (MAttrs.size() || MCPU == "native") {
-    SubtargetFeatures Features;
-
-    // If user asked for the 'native' CPU, we need to autodetect features.
-    // This is necessary for x86 where the CPU might not support all the
-    // features the autodetected CPU name lists in the target. For example,
-    // not all Sandybridge processors support AVX.
-    if (MCPU == "native") {
-      StringMap<bool> HostFeatures;
-      if (sys::getHostCPUFeatures(HostFeatures))
-        for (auto &F : HostFeatures)
-          Features.AddFeature(F.first(), F.second);
-    }
-
-    for (unsigned i = 0; i != MAttrs.size(); ++i)
-      Features.AddFeature(MAttrs[i]);
-    FeaturesStr = Features.getString();
-  }
-
-  if (MCPU == "native")
-    MCPU = sys::getHostCPUName();
-
   return TheTarget->createTargetMachine(TheTriple.getTriple(),
-                                        MCPU, FeaturesStr,
+                                        CPUStr, FeaturesStr,
                                         InitTargetOptionsFromCodeGenFlags(),
                                         RelocModel, CMModel,
                                         GetCodeGenOptLevel());
 }
 
 #ifdef LINK_POLLY_INTO_TOOLS
 namespace polly {
 void initializePollyPasses(llvm::PassRegistry &Registry);
if (NoOutput) {
     Out.reset(new tool_output_file(OutputFilename, EC, sys::fs::F_None));
     if (EC) {
       errs() << EC.message() << '\n';
       return 1;
     }
   }
 
   Triple ModuleTriple(M->getTargetTriple());
+  std::pair<std::string, std::string> CPUFeatures;
   TargetMachine *Machine = nullptr;
-  if (ModuleTriple.getArch())
-    Machine = GetTargetMachine(ModuleTriple);
+  if (ModuleTriple.getArch()) {
+    CPUFeatures = getCPUFeaturesStr();
+    Machine = GetTargetMachine(ModuleTriple, CPUFeatures.first,
+                               CPUFeatures.second);
+  }
+
   std::unique_ptr<TargetMachine> TM(Machine);
 
+  // Override function attributes.
+  overrideFunctionAttributes(CPUFeatures.first, CPUFeatures.second, *M);
+
   // If the output is set to be emitted to standard out, and standard out is a
   // console, print out a warning message and refuse to do it.  We don't
   // impress anyone by spewing tons of binary goo to a terminal.
   if (!Force && !NoOutput && !AnalyzeOnly && !OutputAssembly)
     if (CheckBitcodeOutputToConsole(Out->os(), !Quiet))
       NoOutput = true;
 
   if (PassPipeline.getNumOccurrences() > 0) {