[PATCH] D144596: Add extra parameter to thinlto-prefix-replace

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

In D144596#4176555 <https://reviews.llvm.org/D144596#4176555>, @MaskRay wrote:

> I find the summary difficult to follow and I believe users will be more confusing than me (as a ThinLTO contributor).

I'll take a stab at suggesting a clearer summary in a little bit.

> Feel free to take some of the command examples below to make things clearer
>
> Say, our binary target is named `$package:b` and has a source file `$package/b.cc` along with other dependencies.
>
> The new semantics can be used this way by newer Bazel: `-Wl,--thinlto-index-only=bazel-bin/$package/b-lto-final.params,--thinlto-emit-imports-files,--thinlto-prefix-replace=bazel-bin/;bazel-bin/$package/b.lto/;bazel-bin/$package/b.lto.backend/,--lto-obj-path=bazel-bin/$package/b.lto.merged.o`
>
> - `bazel-bin/$package/_objs/b/b.pic.o`: bitcode file
> - `bazel-bin/$package/_objs.indexing/b/b.pic.o`: minimized bitcode file

To clarify, the above file is currently emitted in the same directory as the full bitcode, and just has a different extension. I suppose you are proposing that we change to the above naming scheme, but that is not so simple, see below.

> - `bazel-bin/$package/b.lto/$package/_objs/b/b.pic.o.thinlto.bc`: index file
> - `bazel-bin/$package/b.lto/$package/_objs/b/b.pic.o.imports`: import file
> - `bazel-bin/$package/b.lto.backend/$package/_objs/b/b.pic.o`: ELF relocatable object file after a backend compile
> - `bazel-bin/$package/b-lto-final.params`: response file for the final native link (it references ELF relocatable object files after backend compiles)
>
> There are two advantages:
>
> - (This doesn't require lld changes.) the full bitcode file and the minimized bitcode file are in different directories. This allows a ThinLTO indexing action to ship fewer input files.

Moving these into separate directories won't affect what the action ships (at least in Bazel). The inputs are specified as full paths, not directories. In fact, the main reason on our end for implementing and using minimized bitcode files was to reduce the aggregate amount of input files sent to the indexing action.

> This may be a Bazel limitation, but a fairly reasonable limitation that other build systems may have as well: they track directories, but not individual files.
>
> - The ELF relocatable object file after a backend compile is now in a different directory. This ensures the directory `b.lto` is only used by one action. This requires this lld patch.
>
>   AIUI `--thinlto-object-suffix-replace=.indexing.o;.o` can effectively be deprecated. So after a transition period when we finally remove `--thinlto-object-suffix-replace`, the feature as proposed in this patch will not increase the overall complexity.

Moving the minimized bitcode to a different directory will not obviate the need for an option. The thin link indexing step needs to know where the corresponding full bitcode is so that it can set up the paths for the later ThinLTO backend actions, which need to know the full IR paths where they are importing from. These full IR bitcode paths are recorded in the .thinlto.bc index files as well as the .imports files. If we moved the minimized bitcode to a different directory but used the same suffix, we would need to replace this with a different option that specified the prefix mapping from the path of the minimized IR (input to the indexing action) to the path of the full IR that will be used during importing by the later ThinLTO backend actions.

> Therefore, I am happy with the direction.





================
Comment at: llvm/include/llvm/LTO/LTO.h:229
                                           std::string NewPrefix,
+                                          std::string NewLinkObjectPrefix,
                                           bool ShouldEmitImportsFiles,
----------------
MaskRay wrote:
> `NewLinkObjectPrefix` refers to the ELF relocatable object file path. Is there a better parameter name to reflect that than `NewLinkObjectPrefix`? @tejohnson 
Maybe FinalObjectPrefix ?


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

https://reviews.llvm.org/D144596