[PATCH] D46034: Support for distributed ThinLTO options

[Rumeet Dhindsa via Phabricator via llvm-commits]

rdhindsa marked 15 inline comments as done.
rdhindsa added inline comments.


================
Comment at: lld/ELF/Driver.cpp:802
+      Config->ThinLTOIndexOnly = true;
+      Config->ThinLTOLinkedObjectsFile =
+          S.substr(strlen("thinlto-index-only="));
----------------
ruiu wrote:
> 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`?
I have updated name to ThinLTOIndexOnlyObjectsFile. This file contains the list of object files. I think keeping objects in the name makes it easy to understand.


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


================
Comment at: lld/ELF/LTO.cpp:90
+         Config->ThinLTOPrefixReplace.find(";") != StringRef::npos);
+  StringRef PrefixReplace = Config->ThinLTOPrefixReplace;
+  std::pair<StringRef, StringRef> Split = PrefixReplace.split(";");
----------------
ruiu wrote:
> Why do you need a temporary variable?
Removed the getThinLTOOldAndNewPrefix function, since the old and new prefix can just be retreived by std::tie(OldPrefix, NewPrefix) = Config->ThinLTOPrefixReplace.split(';');


================
Comment at: lld/ELF/LTO.h:56
   llvm::DenseSet<StringRef> UsedStartStop;
+  llvm::StringMap<bool> ObjectToIndexFileState;
+  std::unique_ptr<llvm::raw_fd_ostream> LinkedObjects;
----------------
ruiu wrote:
> What is this for?
I have removed ObjectToIndexFileState for now, I will be adding it in subsequent patch. It is supposed to track all the object files that are indexed.


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D46034