[PATCH] D147584: [lld] Support separate minimized bitcode file path in --thinlto-prefix-replace

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

Thanks!

> If minimizedbitcodepath is specified it replaces minimizedbitcodepath of the minimized bitcode files with origpath when mapping the minimized bitcode to the full bitcode, otherwise it falls back to the old behavior of assuming both files are in the same directory.

It would be clearer to specify what this rewritten path is used for (i.e. written into the index and imports files for the subsequent distributed ThinLTO backends).

Also, similar to what I suggest below for the variables, origpath is somewhat of a misnomer if we have a minimizedbitcodepath (since the latter is actually the original input path to the thin link). So maybe origpath should be fullbitcodepath?

> With this change, --thinlto-suffix-replace can now be deprecated. Instead of mapping the minimized bitcode to the full bitcode using a suffix replace, we can instead use a prefix replace.

I don't think it should be deprecated until and unless all build systems using it are moved to the new handling. Which might just be bazel but I'm not totally sure? If there are other users they may prefer the existing mechanism and directory structure.



================
Comment at: lld/COFF/Config.h:228
   // thinLTOPrefixReplaceNativeObject, instead of thinLTOPrefixReplaceNew.
+  // If thinLTOPrefixReplaceMinimizedIndex, replace the prefix of minimized
+  // index files, thinLTOPrefixReplaceMinimizedIndex, with
----------------
This should be thinLTOPrefixReplaceMinimizedBitcode not thinLTOPrefixReplaceMinimizedIndex to match the code (and MinimizedBitcode is a better name IMO).

I also think with this change that we should rename thinLTOPrefixReplaceOld to thinLTOPrefixReplaceFullBitcode, and thinLTOPrefixReplaceNew with thinLTOPrefixReplaceOutputFile or something similar, to clarify what each of these paths are.

Then I suggest making the comment something like "replace the prefix of the minimized thin link input bitcode files, thinLTOPrefixReplaceMinimizedBitcode, with thinLTOPrefixReplaceFullBitcode, when the bitcode path is written to output index and imports files, allowing..."


================
Comment at: lld/ELF/Config.h:180
   std::pair<llvm::StringRef, llvm::StringRef> thinLTOObjectSuffixReplace;
   llvm::StringRef thinLTOPrefixReplaceOld;
   llvm::StringRef thinLTOPrefixReplaceNew;
----------------
Similar name changes here and in other Config.h to what I suggested above.

@MaskRay, would it be good to add comments about what these variables are, as is done in the COFF config file? I notice that there are no comments on any of the variables, but I think it might be helpful for these in particular.


================
Comment at: lld/ELF/InputFiles.cpp:1586
     path = replaceThinLTOSuffix(mb.getBufferIdentifier());
+    if (!config->thinLTOPrefixReplaceMinimizedBitcode.empty()) {
+      path = lto::getThinLTOOutputFile(
----------------
The way the code is written, both suffix and prefix replacing can be done. If we don't want people to rely on that, and want to eventually move off of suffix replacement, we probably should give an error somewhere, i.e. in the option parsing, if both are specified. Then maybe add a comment here that we have already checked that only one is in effect.


================
Comment at: lld/ELF/InputFiles.cpp:1587
+    if (!config->thinLTOPrefixReplaceMinimizedBitcode.empty()) {
+      path = lto::getThinLTOOutputFile(
+          path, config->thinLTOPrefixReplaceMinimizedBitcode.str(),
----------------
The name getThinLTOOutputFile is no longer very accurate. Perhaps make this getThinLTOReplacePathPrefix or something?


================
Comment at: llvm/tools/gold/gold-plugin.cpp:897
         // TODO: Add support for optional native object path in
-        // thinlto_prefix_replace option to match lld.
+        //  thinlto_prefix_replace option to match lld.
         /*NativeObjectPrefix=*/"", options::thinlto_emit_imports_files,
----------------
Remove the extra space added here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147584