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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

Sorry for the delay. I finished a long trip and was spending some time to clean up my email queue:)

> Currently, the --thinlto-suffix-replace="oldsuffix;newsuffix" option is used during distributed ThinLTO thin links to specify the mapping...

I feel that this paragraph can be made clearer. How about this:

For distributed ThinLTO, `--thinlto-index-only=` emits index files (`*.thinlink.bc`) and import files (`*.imports`) that reference IR module names.
When minimized bitcode files like `a.indexing.o` are used as input, we use `--thinlto-suffix-replace='.indexing.o;.o'` so that the IR module names record `a.o` instead of `a.indexing.o`.
Backend compiles need the full bitcode file paths, not the minimized bitcode file paths.

However, this forces the full bitcode file and its corresponding minimized bitcode file to be in the same directory.

This patch adds the fourth component to the value of --thinlto-prefix-replace=
to allow a separate directory for minimized bitcode file.

> This is important to support builds and build systems where a directory that is the output of a single action must either be fully included or not included as the input of another action (e.g. the thin link, it must take as its input the minimized bitcode files but not take as input the full bitcode files).

This paragraph can change the subject to be clearer.

In Bazel, when we have an action that may take an tree artifact as an input, the action takes as input all the files inside the directory of the tree artifact or none of them.
If a full bitcode file and its corresponding minimized bitcode file within the same directory, Bazel will have to ship both files to a backend compile action, causing waste.

---

The error message is

`error: --thinlto-prefix-replace=old_dir;new_dir;obj_dir must be used with --thinlto-index-only=`

It can be changed to:

`error: --thinlto-prefix-replace=full;index;obj;minimized when obj is non-empty must be used with --thinlto-index-only=`



================
Comment at: lld/test/COFF/thinlto-minimized-bitcode-prefix-replace.ll:12
+;; resulting index.
+; RUN: lld-link -entry:main -thinlto-index-only -thinlto-prefix-replace:"%t/full;%t/indexing" %t/full/1.obj -out:%t/t.exe
+; RUN: cp %t/indexing/1.obj.thinlto.bc %t/indexing/1.obj.thinlto.bc.orig
----------------
Bazel currently uses `a.indexing.o` for minimized bitcode files. Using `%t/indexing` will cause confusion for the emitted index files. Suggest: `%t/index`

Also fix tests  for other ports.


================
Comment at: lld/test/COFF/thinlto-minimized-bitcode-prefix-replace.ll:23
+;; Make sure the third string to thinlto-prefix-replace does not affect the test
+; RUN: lld-link -entry:main -thinlto-index-only:"%t/unused" -thinlto-prefix-replace:"%t/full;%t/indexing;%t/unused;%t/minimized" \
+; RUN:     %t/minimized/1.obj -out:%t/t.exe
----------------
Using `%t/unused` as both the output response file and the directory in `-thinlto-prefix-replace:` leads to confusion.

Suggest `-thinlto-index-only:%t/a.rsp -thinlto-prefix-replace:'%t/full;%t/index;%t/obj;%t/minimized'`

Prefer single quotes to double quotes.

Add a RUN line to inspect `FileCheck --check-prefix=xxx < %t/a.rsp`. The output is useful.


================
Comment at: lld/test/COFF/thinlto-minimized-bitcode-prefix-replace.ll:39
+!llvm.module.flags = !{!1}
\ No newline at end of file

----------------
Fix: No newline at end of file


================
Comment at: lld/test/ELF/lto/thinlto-minimized-bitcode-prefix-replace.ll:1
+; REQUIRES: x86
+;; Test to make sure the thinlto-object-suffix-replace option is handled
----------------
This file just duplicates `thinlto-object-suffix-replace.ll` ?


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

https://reviews.llvm.org/D147584