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

Thu Mar 9 12:53:36 PST 2023

MaskRay added a comment.

In D144596#4178243 <https://reviews.llvm.org/D144596#4178243>, @tejohnson wrote:

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

Thanks!

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

The "To support tree artifacts with an unknown number of full bitcode files and minimized bitcode files, we need to use different directories." part in my previous long reply captures my understanding.

Yes, `b.pic.o` and `b.pic.indexing.o` are in the same directory, and we use `--thinlto-object-suffix-replace='.indexing.o;.o'` to derive the full bitcode file from the minimized bitcode file.
With off-topic discussion with @itf , I think I kinda understand why `b.pic.o` and `b.pic.indexing.o` need to be in different directories now (with tree artifacts we have to ship the whole directory, not individual files), but more context from the Bazel side will be nice to have.

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

Agreed. `--thinlto-prefix-replace='thin/;index/;obj/;indexing/'` in my previous long message should work.

>> Therefore, I am happy with the direction.

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

https://reviews.llvm.org/D144596