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 ↗

(On Diff #25080)

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 ↗

(On Diff #25080)

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?

llvm/trunk/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/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>
static inline std::string getFeaturesStr() {
   }
 
   for (unsigned i = 0; i != MAttrs.size(); ++i)
     Features.AddFeature(MAttrs[i]);
 
   return 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

llvm/trunk/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;
 }
 
+/// \brief Overwrite attribute Kind in function F.
+void overrideFunctionAttribute(StringRef Kind, StringRef Value, Function &F);
+
 } // End llvm namespace
 
 #endif

llvm/trunk/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);
+}

llvm/trunk/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 that llc can overide function attributes target-cpu and target-features
+; using command line options -mcpu and -mattr.
+
+; 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" }

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

+; RUN: opt < %s -mtriple=x86_64-apple-darwin -mcpu=broadwell -mattr=+avx2 -S | FileCheck %s
+
+; Check that opt can rewrite function attributes target-cpu and target-features
+; using command line options -mcpu and -mattr.
+
+; 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" }

llvm/trunk/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 }
 

llvm/trunk/tools/llc/llc.cpp

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(CPUStr, FeaturesStr, *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;

llvm/trunk/tools/opt/opt.cpp

#endif
   if (ModuleTriple.getArch()) {
     CPUStr = getCPUStr();
     FeaturesStr = getFeaturesStr();
     Machine = GetTargetMachine(ModuleTriple, CPUStr, FeaturesStr);
   }
 
   std::unique_ptr<TargetMachine> TM(Machine);
 
+  // Override function attributes.
+  overrideFunctionAttributes(CPUStr, FeaturesStr, *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) {