[PATCH] D155326: [Clang][lld][RISCV] Passing 'mattr' to lld/llvmgold

[Yingwei Zheng via Phabricator via cfe-commits]

dtcxzyw created this revision.
dtcxzyw added reviewers: MaskRay, jrtc27, kito-cheng, craig.topper.
dtcxzyw added projects: lld, clang.
Herald added subscribers: jobnoorman, VincentWu, vkmr, luismarques, 
sameer.abuasal, s.egerton, Jim, benna, psnobl, PkmX, rogfer01, shiva0217, 
simoncook, arichardson, emaste.
Herald added a project: All.
dtcxzyw requested review of this revision.
Herald added subscribers: llvm-commits, cfe-commits, wangpc, eopXD.
Herald added a project: LLVM.

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 .


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D155326

Files:
  clang/lib/Driver/ToolChains/Arch/CSKY.cpp
  clang/lib/Driver/ToolChains/Arch/CSKY.h
  clang/lib/Driver/ToolChains/CommonArgs.cpp
  lld/ELF/Driver.cpp
  lld/ELF/Options.td
  llvm/tools/gold/gold-plugin.cpp

Index: llvm/tools/gold/gold-plugin.cpp
===
--- llvm/tools/gold/gold-plugin.cpp
+++ llvm/tools/gold/gold-plugin.cpp
@@ -33,6 +33,7 @@
 #include "llvm/Support/Threading.h"
 #include "llvm/Support/raw_ostream.h"
 #include "llvm/TargetParser/Host.h"
+#include "llvm/TargetParser/SubtargetFeature.h"
 #include 
 #include 
 #include 
@@ -152,6 +153,7 @@
   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.
@@ -228,6 +230,8 @@
 
 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=")) {
@@ -875,7 +879,8 @@
   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 CGOptLevelOrNone =
Index: lld/ELF/Options.td
===
--- lld/ELF/Options.td
+++ lld/ELF/Options.td
@@ -656,6 +656,7 @@
 def: J<"plugin-opt=jobs=">, Alias, HelpText<"Alias for --thinlto-jobs=">;
 def: J<"plugin-opt=lto-partitions=">, Alias, 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, HelpText<"Alias for --lto-cs-profile-generate">;
 def: J<"plugin-opt=cs-profile-path=">,
Index: lld/ELF/Driver.cpp
===
--- lld/ELF/Driver.cpp
+++ lld/ELF/Driver.cpp
@@ -1467,6 +1467,10 @@
 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());
 
Index: clang/lib/Driver/ToolChains/CommonArgs.cpp
===
--- clang/lib/Driver/ToolChains/CommonArgs.cpp
+++ clang/lib/Driver/ToolChains/CommonArgs.cpp
@@ -62,6 +62,7 @@
 #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 
 
@@ -485,10 +486,10 @@
 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 Features;
+static void collectTargetFeatures(const Driver &D, const llvm::Triple &Triple,
+  const ArgList &Args,
+  std::vector &Features,
+  bool ForAS) {
   switch (Triple.getArch()) {
   default:
 break;
@@ -556,13 +557,20 @@
 ve::getVETargetFeatures(D, Args, Features);
 

[PATCH] D155326: [Clang][lld][RISCV] Passing 'mattr' to lld/llvmgold

[Yingwei Zheng via Phabricator via cfe-commits]

dtcxzyw added a comment.

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).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155326/new/

https://reviews.llvm.org/D155326

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146364: [Driver] Fix naming conflicts of getStatsFileName when using LTO

[Yingwei Zheng via Phabricator via cfe-commits]

dtcxzyw created this revision.
dtcxzyw added reviewers: liaolucy, StephenFan.
Herald added a subscriber: inglorion.
Herald added a project: All.
dtcxzyw requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay.
Herald added a project: clang.

When using `clang` with `-flto=thin -save-stats=obj` to compile a multi-file 
program, `clang` will save internal statistics during the link time code 
generation to `/.stats` because `BaseName` is empty. When multiple 
binaries are placed in the same directory, conflicts will be caused by 
identical filenames for statistics. This patch uses the output filename as the 
base name instead of the input when `-save-stats=obj`.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D146364

Files:
  clang/lib/Driver/ToolChains/CommonArgs.cpp


Index: clang/lib/Driver/ToolChains/CommonArgs.cpp
===
--- clang/lib/Driver/ToolChains/CommonArgs.cpp
+++ clang/lib/Driver/ToolChains/CommonArgs.cpp
@@ -1774,14 +1774,13 @@
 StringRef SaveStats = A->getValue();
 if (SaveStats == "obj" && Output.isFilename()) {
   StatsFile.assign(Output.getFilename());
-  llvm::sys::path::remove_filename(StatsFile);
-} else if (SaveStats != "cwd") {
+} else if (SaveStats == "cwd") {
+  StatsFile.assign(Input.getBaseInput());
+} else {
   D.Diag(diag::err_drv_invalid_value) << A->getAsString(Args) << SaveStats;
   return {};
 }
 
-StringRef BaseName = llvm::sys::path::filename(Input.getBaseInput());
-llvm::sys::path::append(StatsFile, BaseName);
 llvm::sys::path::replace_extension(StatsFile, "stats");
   } else {
 assert(D.CCPrintInternalStats);


Index: clang/lib/Driver/ToolChains/CommonArgs.cpp
===
--- clang/lib/Driver/ToolChains/CommonArgs.cpp
+++ clang/lib/Driver/ToolChains/CommonArgs.cpp
@@ -1774,14 +1774,13 @@
 StringRef SaveStats = A->getValue();
 if (SaveStats == "obj" && Output.isFilename()) {
   StatsFile.assign(Output.getFilename());
-  llvm::sys::path::remove_filename(StatsFile);
-} else if (SaveStats != "cwd") {
+} else if (SaveStats == "cwd") {
+  StatsFile.assign(Input.getBaseInput());
+} else {
   D.Diag(diag::err_drv_invalid_value) << A->getAsString(Args) << SaveStats;
   return {};
 }
 
-StringRef BaseName = llvm::sys::path::filename(Input.getBaseInput());
-llvm::sys::path::append(StatsFile, BaseName);
 llvm::sys::path::replace_extension(StatsFile, "stats");
   } else {
 assert(D.CCPrintInternalStats);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146364: [Driver] Fix naming conflicts of getStatsFileName when using LTO

[Yingwei Zheng via Phabricator via cfe-commits]

dtcxzyw updated this revision to Diff 506387.
dtcxzyw removed subscribers: MaskRay, cfe-commits, inglorion.
dtcxzyw added a comment.

Fix test errors.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146364/new/

https://reviews.llvm.org/D146364

Files:
  clang/lib/Driver/ToolChains/CommonArgs.cpp


Index: clang/lib/Driver/ToolChains/CommonArgs.cpp
===
--- clang/lib/Driver/ToolChains/CommonArgs.cpp
+++ clang/lib/Driver/ToolChains/CommonArgs.cpp
@@ -1774,14 +1774,13 @@
 StringRef SaveStats = A->getValue();
 if (SaveStats == "obj" && Output.isFilename()) {
   StatsFile.assign(Output.getFilename());
-  llvm::sys::path::remove_filename(StatsFile);
-} else if (SaveStats != "cwd") {
+} else if (SaveStats == "cwd") {
+  StatsFile.assign(llvm::sys::path::filename(Input.getBaseInput()));
+} else {
   D.Diag(diag::err_drv_invalid_value) << A->getAsString(Args) << SaveStats;
   return {};
 }
 
-StringRef BaseName = llvm::sys::path::filename(Input.getBaseInput());
-llvm::sys::path::append(StatsFile, BaseName);
 llvm::sys::path::replace_extension(StatsFile, "stats");
   } else {
 assert(D.CCPrintInternalStats);


Index: clang/lib/Driver/ToolChains/CommonArgs.cpp
===
--- clang/lib/Driver/ToolChains/CommonArgs.cpp
+++ clang/lib/Driver/ToolChains/CommonArgs.cpp
@@ -1774,14 +1774,13 @@
 StringRef SaveStats = A->getValue();
 if (SaveStats == "obj" && Output.isFilename()) {
   StatsFile.assign(Output.getFilename());
-  llvm::sys::path::remove_filename(StatsFile);
-} else if (SaveStats != "cwd") {
+} else if (SaveStats == "cwd") {
+  StatsFile.assign(llvm::sys::path::filename(Input.getBaseInput()));
+} else {
   D.Diag(diag::err_drv_invalid_value) << A->getAsString(Args) << SaveStats;
   return {};
 }
 
-StringRef BaseName = llvm::sys::path::filename(Input.getBaseInput());
-llvm::sys::path::append(StatsFile, BaseName);
 llvm::sys::path::replace_extension(StatsFile, "stats");
   } else {
 assert(D.CCPrintInternalStats);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits