[PATCH] D46034: Support for distributed ThinLTO options

Rui Ueyama via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Apr 24 16:36:48 PDT 2018


ruiu added a comment.

There are a few things to note:

1. When you upload a patch, please include all context. Are you using git? If so, use `git diff -u 10000` to include virtually the entire file in your diff.
2. Please read the LLVM coding style and stick with it. The easiest way of doing it is to use clang-format-diff to automatically format your patch according to the LLVM coding style.



================
Comment at: lld/ELF/Driver.cpp:798
       Config->LTOSampleProfile = S.substr(strlen("sample-profile="));
-    else if (!S.startswith("/") && !S.startswith("-fresolution=") &&
+    else if (S == "thinlto-index-only")
+      Config->ThinLTOIndexOnly = true;
----------------
Please append {} to all these `if ~ else if ~ else`.


================
Comment at: lld/ELF/Driver.cpp:802
+      Config->ThinLTOIndexOnly = true;
+      Config->ThinLTOLinkedObjectsFile =
+          S.substr(strlen("thinlto-index-only="));
----------------
Basically all members in Config class are named after their corresponding command line options. This is a special case, but I'm reluctant to give a totally different name than the corresponding option. How about `ThinLTOIndexOnlyFile`?


================
Comment at: lld/ELF/Driver.cpp:803
+      Config->ThinLTOLinkedObjectsFile =
+          S.substr(strlen("thinlto-index-only="));
+    } else if (S.startswith("thinlto-prefix-replace=")) {
----------------
I'd just write 19 instead of strlen(...).


================
Comment at: lld/ELF/Driver.cpp:806
+      Config->ThinLTOPrefixReplace =
+          S.substr(strlen("thinlto-prefix-replace="));
+      if (Config->ThinLTOPrefixReplace.find(';') == std::string::npos)
----------------
Ditto


================
Comment at: lld/ELF/Driver.cpp:807
+          S.substr(strlen("thinlto-prefix-replace="));
+      if (Config->ThinLTOPrefixReplace.find(';') == std::string::npos)
+        fatal("thinlto-prefix-replace expects 'old;new' format");
----------------
Use StringRef::contains.


================
Comment at: lld/ELF/Driver.cpp:808
+      if (Config->ThinLTOPrefixReplace.find(';') == std::string::npos)
+        fatal("thinlto-prefix-replace expects 'old;new' format");
+    } else if (!S.startswith("/") && !S.startswith("-fresolution=") &&
----------------
fatal() is not for reporting a wrong usage. Use error() instead.


================
Comment at: lld/ELF/Driver.cpp:1015
     case OPT_start_lib:
-      if (InLib)
-        error("nested --start-lib");
-      if (InputFile::IsInGroup)
-        error("may not nest --start-lib in --start-group");
-      InLib = true;
-      InputFile::IsInGroup = true;
+      // ThinLTO needs to generate object files for all bitcode files
+      if (!Config->ThinLTOIndexOnly) {
----------------
I do understand that you need this or otherwise ThinLTO doesn't work. But this comment doesn't explain why we really need this. Could you expand the comment so that why ThinLTO needs to see all bitcode files even within --start-lib/end-lib?


================
Comment at: lld/ELF/Driver.cpp:1016
+      // ThinLTO needs to generate object files for all bitcode files
+      if (!Config->ThinLTOIndexOnly) {
+        if (InLib) 
----------------
Flip the condition and break early.

  if (Config->ThinLTOIndexOnly)
    break;



================
Comment at: lld/ELF/LTO.cpp:72
+// linking of distributed ThinLTO.
+static std::unique_ptr<raw_fd_ostream> CreateLinkedObjectsFile() {
+  if (Config->ThinLTOLinkedObjectsFile.empty()) return nullptr;
----------------
Please stick with the LLVM/lld function naming scheme.



================
Comment at: lld/ELF/LTO.cpp:73
+static std::unique_ptr<raw_fd_ostream> CreateLinkedObjectsFile() {
+  if (Config->ThinLTOLinkedObjectsFile.empty()) return nullptr;
+  assert(Config->ThinLTOIndexOnly);
----------------
Please use clang-format.



================
Comment at: lld/ELF/LTO.cpp:74
+  if (Config->ThinLTOLinkedObjectsFile.empty()) return nullptr;
+  assert(Config->ThinLTOIndexOnly);
+  std::error_code EC;
----------------
I don't think you need this assertion.


================
Comment at: lld/ELF/LTO.cpp:88-89
+                                      std::string &NewPrefix) {
+  assert(Config->ThinLTOPrefixReplace.empty() ||
+         Config->ThinLTOPrefixReplace.find(";") != StringRef::npos);
+  StringRef PrefixReplace = Config->ThinLTOPrefixReplace;
----------------
Ditto


================
Comment at: lld/ELF/LTO.cpp:90
+         Config->ThinLTOPrefixReplace.find(";") != StringRef::npos);
+  StringRef PrefixReplace = Config->ThinLTOPrefixReplace;
+  std::pair<StringRef, StringRef> Split = PrefixReplace.split(";");
----------------
Why do you need a temporary variable?


================
Comment at: lld/ELF/LTO.cpp:101
+static std::unique_ptr<lto::LTO> createLTO(llvm::lto::IndexWriteCallback OnIndexWrite,
+                                      raw_fd_ostream *LinkedObjectsFile) {
   lto::Config Conf;
----------------
Format


================
Comment at: lld/ELF/LTO.cpp:142
+    std::string OldPrefix, NewPrefix;
+    getThinLTOOldAndNewPrefix(OldPrefix, NewPrefix);
+    Backend = llvm::lto::createWriteIndexesThinBackend(
----------------
You can replace this with

  std::string Old;
  std::string New;
  std::tie(Old, New) = Config->ThinLTOPrefixReplace.split(';');


================
Comment at: lld/ELF/LTO.cpp:156
+BitcodeCompiler::BitcodeCompiler() {
+  if (Config->ThinLTOIndexOnly) LinkedObjects = CreateLinkedObjectsFile();
+  LTOObj = createLTO(
----------------
Please follow the LLVM coding style.


================
Comment at: lld/ELF/LTO.cpp:158
+  LTOObj = createLTO(
+      [&](const std::string &Identifier) {
+        ObjectToIndexFileState[Identifier] = true;
----------------
Please fix style.


================
Comment at: lld/ELF/LTO.cpp:270
 
+  if (Config->ThinLTOIndexOnly) exit(0);
+
----------------
Well, you need to use clang-format.


================
Comment at: lld/ELF/LTO.h:56
   llvm::DenseSet<StringRef> UsedStartStop;
+  llvm::StringMap<bool> ObjectToIndexFileState;
+  std::unique_ptr<llvm::raw_fd_ostream> LinkedObjects;
----------------
What is this for?


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D46034





More information about the llvm-commits mailing list