[Clang][lld][RISCV] Passing 'mattr' to lld/llvmgold

[dtcxzyw]

Summary

When building applications with LTO/ThinLTO, lld and llvmgold emit wrong .riscv.attributes sections due to the lack of sub-target features (codegen::getMAttrs() returns empty).
This patch adds a new plugin option mattr for lld and llvmgold, to get right module-level sub-target features for temporary ELF objects generated from llvm bitcode.

I think it is a better solution than merging func-level sub-target features like D142191.

[jrtc27]

Why do you believe this is better than encoding it in the module's IR like the ABI?

[dtcxzyw]

In D155326#4501997, @jrtc27 wrote:

Why do you believe this is better than encoding it in the module's IR like the ABI?

• There is no module-level metadata for target CPU and sub-target features. So I just think that this patch is better than merging from func-level attributes like D142191.
• Clang passes -plugin-opt=mcpu to lld. Then lld builds lto::Config using codegen::getCPUStr and codegen::getMAttrs (both get the result from command args). It is taken for granted that clang passes -plugin-opt=mattr down to lld.

If we introduce top-level fields like target cpu and target features, we should modify a lot of things (clang/llvm/lld). It can take a long time to migrate (like opaque pointers).

[craig.topper]

-mattr is a different syntax than -march that users are used to. Why would we want to expose that? There was previously a proposal for doing this automatically https://reviews.llvm.org/D132843

[craig.topper]

In D155326#4502097, @dtcxzyw wrote:

In D155326#4501997, @jrtc27 wrote:

Why do you believe this is better than encoding it in the module's IR like the ABI?

• There is no module-level metadata for target CPU and sub-target features. So I just think that this patch is better than merging from func-level attributes like D142191.
• Clang passes -plugin-opt=mcpu to lld. Then lld builds lto::Config using codegen::getCPUStr and codegen::getMAttrs (both get the result from command args). It is taken for granted that clang passes -plugin-opt=mattr down to lld.

If we introduce top-level fields like target cpu and target features, we should modify a lot of things (clang/llvm/lld). It can take a long time to migrate (like opaque pointers).

You're right it might take time, but we've been saying that's what needs to be done for a year. It might have been done by now if the work had gotten started.

clang/lib/Driver/ToolChains/Arch/CSKY.h

enum class FloatABI {
   SoftFP,
   Hard,
 };
 
 FloatABI getCSKYFloatABI(const Driver &D, const llvm::opt::ArgList &Args);
 
 void getCSKYTargetFeatures(const Driver &D, const llvm::Triple &Triple,
                            const llvm::opt::ArgList &Args,
-                           llvm::opt::ArgStringList &CmdArgs,
                            std::vector<llvm::StringRef> &Features);
 
 std::optional<llvm::StringRef> getCSKYArchName(const Driver &D,
                                                const llvm::opt::ArgList &Args,
                                                const llvm::Triple &Triple);
 
 } // end namespace csky
 } // namespace tools
 } // end namespace driver
 } // end namespace clang
 
 #endif // LLVM_CLANG_LIB_DRIVER_TOOLCHAINS_ARCH_CSKY_H

clang/lib/Driver/ToolChains/Arch/CSKY.cpp

if (!llvm::CSKY::getFPUFeatures(FPUID, Features)) {
     D.Diag(clang::diag::err_drv_clang_unsupported) << A->getAsString(Args);
     return llvm::CSKY::FK_INVALID;
   }
 
   return FPUID;
 }
 
 void csky::getCSKYTargetFeatures(const Driver &D, const llvm::Triple &Triple,
-                                 const ArgList &Args, ArgStringList &CmdArgs,
+                                 const ArgList &Args,
                                  std::vector<llvm::StringRef> &Features) {
   llvm::StringRef archName;
   llvm::StringRef cpuName;
   llvm::CSKY::ArchKind ArchKind = llvm::CSKY::ArchKind::INVALID;
   if (const Arg *A = Args.getLastArg(options::OPT_march_EQ)) {
     ArchKind = llvm::CSKY::parseArch(A->getValue());
     if (ArchKind == llvm::CSKY::ArchKind::INVALID) {
       D.Diag(clang::diag::err_drv_invalid_arch_name) << A->getAsString(Args);

clang/lib/Driver/ToolChains/CommonArgs.cpp

 #include "llvm/Support/Path.h"
 #include "llvm/Support/Process.h"
 #include "llvm/Support/Program.h"
 #include "llvm/Support/ScopedPrinter.h"
 #include "llvm/Support/Threading.h"
 #include "llvm/Support/VirtualFileSystem.h"
 #include "llvm/Support/YAMLParser.h"
 #include "llvm/TargetParser/Host.h"
+#include "llvm/TargetParser/SubtargetFeature.h"
 #include "llvm/TargetParser/TargetParser.h"
 #include <optional>
 
 using namespace clang::driver;
 using namespace clang::driver::tools;
 using namespace clang;
 using namespace llvm::opt;
 

 static void getWebAssemblyTargetFeatures(const Driver &D,
                                          const llvm::Triple &Triple,
                                          const ArgList &Args,
                                          std::vector<StringRef> &Features) {
   handleTargetFeaturesGroup(D, Triple, Args, Features,
                             options::OPT_m_wasm_Features_Group);
 }
 
-void tools::getTargetFeatures(const Driver &D, const llvm::Triple &Triple,
-                              const ArgList &Args, ArgStringList &CmdArgs,
-                              bool ForAS, bool IsAux) {
-  std::vector<StringRef> Features;
+static void collectTargetFeatures(const Driver &D, const llvm::Triple &Triple,
+                                  const ArgList &Args,
+                                  std::vector<StringRef> &Features,
+                                  bool ForAS) {
   switch (Triple.getArch()) {
   default:
     break;
   case llvm::Triple::mips:
   case llvm::Triple::mipsel:
   case llvm::Triple::mips64:
   case llvm::Triple::mips64el:
     mips::getMIPSTargetFeatures(D, Triple, Args, Features);
case llvm::Triple::m68k:
     break;
   case llvm::Triple::msp430:
     msp430::getMSP430TargetFeatures(D, Args, Features);
     break;
   case llvm::Triple::ve:
     ve::getVETargetFeatures(D, Args, Features);
     break;
   case llvm::Triple::csky:
-    csky::getCSKYTargetFeatures(D, Triple, Args, CmdArgs, Features);
+    csky::getCSKYTargetFeatures(D, Triple, Args, Features);
     break;
   case llvm::Triple::loongarch32:
   case llvm::Triple::loongarch64:
     loongarch::getLoongArchTargetFeatures(D, Triple, Args, Features);
     break;
   }
+}
+
+void tools::getTargetFeatures(const Driver &D, const llvm::Triple &Triple,
+                              const ArgList &Args, ArgStringList &CmdArgs,
+                              bool ForAS, bool IsAux) {
+  std::vector<StringRef> Features;
+  collectTargetFeatures(D, Triple, Args, Features, ForAS);
 
   for (auto Feature : unifyTargetFeatures(Features)) {
     CmdArgs.push_back(IsAux ? "-aux-target-feature" : "-target-feature");
     CmdArgs.push_back(Feature.data());
   }
 }
 
 llvm::StringRef tools::getLTOParallelism(const ArgList &Args, const Driver &D) {
#endif
   // the plugin.
 
   // Handle flags for selecting CPU variants.
   std::string CPU = getCPUName(D, Args, ToolChain.getTriple());
   if (!CPU.empty())
     CmdArgs.push_back(
         Args.MakeArgString(Twine(PluginOptPrefix) + ExtraDash + "mcpu=" + CPU));
 
+  std::vector<StringRef> Features;
+  collectTargetFeatures(D, ToolChain.getTriple(), Args, Features,
+                        /*ForAS*/ false);
+  if (!Features.empty()) {
+    llvm::SubtargetFeatures FeatureStr;
+    for (auto Feature : unifyTargetFeatures(Features))
+      FeatureStr.AddFeature(Feature);
+    CmdArgs.push_back(Args.MakeArgString(Twine(PluginOptPrefix) + ExtraDash +
+                                         "mattr=" + FeatureStr.getString()));
+  }
+
   if (Arg *A = Args.getLastArg(options::OPT_O_Group)) {
     // The optimization level matches
     // CompilerInvocation.cpp:getOptimizationLevel().
     StringRef OOpt;
     if (A->getOption().matches(options::OPT_O4) ||
         A->getOption().matches(options::OPT_Ofast))
       OOpt = "3";
     else if (A->getOption().matches(options::OPT_O)) {

lld/ELF/Driver.cpp

static void readConfigs(opt::InputArgList &args) {
 
   cl::ResetAllOptionOccurrences();
 
   // Parse LTO options.
   if (auto *arg = args.getLastArg(OPT_plugin_opt_mcpu_eq))
     parseClangOption(saver().save("-mcpu=" + StringRef(arg->getValue())),
                      arg->getSpelling());
 
+  if (auto *arg = args.getLastArg(OPT_plugin_opt_mattr_eq))
+    parseClangOption(saver().save("-mattr=" + StringRef(arg->getValue())),
+                     arg->getSpelling());
+
   for (opt::Arg *arg : args.filtered(OPT_plugin_opt_eq_minus))
     parseClangOption(std::string("-") + arg->getValue(), arg->getSpelling());
 
   // GCC collect2 passes -plugin-opt=path/to/lto-wrapper with an absolute or
   // relative path. Just ignore. If not ended with "lto-wrapper" (or
   // "lto-wrapper.exe" for GCC cross-compiled for Windows), consider it an
   // unsupported LLVMgold.so option and error.
   for (opt::Arg *arg : args.filtered(OPT_plugin_opt_eq)) {

lld/ELF/Options.td

 def plugin_opt_dwo_dir_eq: J<"plugin-opt=dwo_dir=">,
   HelpText<"Directory to store .dwo files when LTO and debug fission are used">;
 def plugin_opt_emit_asm: F<"plugin-opt=emit-asm">,
   Alias<lto_emit_asm>, HelpText<"Alias for --lto-emit-asm">;
 def plugin_opt_emit_llvm: F<"plugin-opt=emit-llvm">;
 def: J<"plugin-opt=jobs=">, Alias<thinlto_jobs_eq>, HelpText<"Alias for --thinlto-jobs=">;
 def: J<"plugin-opt=lto-partitions=">, Alias<lto_partitions>, HelpText<"Alias for --lto-partitions">;
 def plugin_opt_mcpu_eq: J<"plugin-opt=mcpu=">;
+def plugin_opt_mattr_eq: J<"plugin-opt=mattr=">;
 def: F<"plugin-opt=cs-profile-generate">,
   Alias<lto_cs_profile_generate>, HelpText<"Alias for --lto-cs-profile-generate">;
 def: J<"plugin-opt=cs-profile-path=">,
   Alias<lto_cs_profile_file>, HelpText<"Alias for --lto-cs-profile-file">;
 def: J<"plugin-opt=obj-path=">,
   Alias<lto_obj_path_eq>,
   HelpText<"Alias for --lto-obj-path=">;
 def plugin_opt_opaque_pointers: F<"plugin-opt=opaque-pointers">,

llvm/tools/gold/gold-plugin.cpp

 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/ManagedStatic.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/TargetSelect.h"
 #include "llvm/Support/Threading.h"
 #include "llvm/Support/raw_ostream.h"
 #include "llvm/TargetParser/Host.h"
+#include "llvm/TargetParser/SubtargetFeature.h"
 #include <list>
 #include <map>
 #include <plugin-api.h>
 #include <string>
 #include <system_error>
 #include <utility>
 #include <vector>
 
#ifdef NDEBUG
   static bool DisableVerify = true;
 #else
   static bool DisableVerify = false;
 #endif
   static std::string obj_path;
   static std::string extra_library_path;
   static std::string triple;
   static std::string mcpu;
+  static std::string mattr;
   // When the thinlto plugin option is specified, only read the function
   // the information from intermediate files and write a combined
   // global index for the ThinLTO backends.
   static bool thinlto = false;
   // If false, all ThinLTO backend compilations through code gen are performed
   // using multiple threads in the gold-plugin, before handing control back to
   // gold. If true, write individual backend index files which reflect
   // the import decisions, and exit afterwards. The assumption is
#endif
   static void process_plugin_option(const char *opt_)
   {
     if (opt_ == nullptr)
       return;
     llvm::StringRef opt = opt_;
 
     if (opt.consume_front("mcpu=")) {
       mcpu = std::string(opt);
+    } else if (opt.consume_front("mattr=")) {
+      mattr = std::string(opt);
     } else if (opt.consume_front("extra-library-path=")) {
       extra_library_path = std::string(opt);
     } else if (opt.consume_front("mtriple=")) {
       triple = std::string(opt);
     } else if (opt.consume_front("obj-path=")) {
       obj_path = std::string(opt);
     } else if (opt == "emit-llvm") {
       TheOutputType = OT_BC_ONLY;
static std::unique_ptr<LTO> createLTO(IndexWriteCallback OnIndexWrite,
   Conf.Options.RelaxELFRelocations = false;
 
   // Toggle function/data sections.
   if (!codegen::getExplicitFunctionSections())
     Conf.Options.FunctionSections = SplitSections;
   if (!codegen::getExplicitDataSections())
     Conf.Options.DataSections = SplitSections;
 
-  Conf.MAttrs = codegen::getMAttrs();
+  SubtargetFeatures Features(options::mattr);
+  Conf.MAttrs = Features.getFeatures();
   Conf.RelocModel = RelocationModel;
   Conf.CodeModel = codegen::getExplicitCodeModel();
   std::optional<CodeGenOpt::Level> CGOptLevelOrNone =
       CodeGenOpt::getLevel(options::OptLevel);
   assert(CGOptLevelOrNone && "Invalid optimization level");
   Conf.CGOptLevel = *CGOptLevelOrNone;
   Conf.DisableVerify = options::DisableVerify;
   Conf.OptLevel = options::OptLevel;