Address the most important (decided by me) of Teresa's DTLTO review comments #2
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This will be used for supplying a `-target=<triple>` option to clang for DTLTO. Note that If DTLTO is changed to use an optimisation tool that does not require an explicit triple to be passed then the triple handling can be removed entirely.
The DTLTO ThinLTO backend will override this function to perform code generation. The DTLTO ThinLTO backend will not do any codegen when invoked for each task. Instead, it will generate the required information (e.g., the summary index shard, import list, etc.) to allow for the codegen to be performed externally. The backend's `wait` function will then invoke an external distributor process to do backend compilations.
The DTLTO ThinLTO backend will need to customize the file paths used for the summary index shard files. This is so that the names of these files can include a unique ID so that multiple DTLTO links do not overwrite each other's summary index shard files. It also needs to be able to get the import files list for each job in memory rather than writing it to a file. This is to allow the import lists to be included in the JSON used to communicate with the external distributor process.
The new setup() member is now called to allow the ThinLTO backends to prepare for code generation. For the DTLTO backend, this will be used to preallocate storage for the information required to perform the backend compilation jobs.
Move some setup code and state to a base class. This will allow the DTLTO ThinLTO backend to share this setup code and state by also deriving from this base class.
Structural changes: 1. A new ThinLTO backend implementing DTLTO has been added. 2. Both the new backend and the InProcess backend derive from a common base class to share common setup code and state. 3. The target triple is now stored for the ThinLTO bitcode files. 4. A new setup() member is called, which the ThinLTO backends can use to prepare for code generation. For the DTLTO backend, this is used to pre-allocate storage for the information required to perform the backend compilation jobs. 5. The functions for emitting summary index shard and imports files have been altered to allow the caller to specify the filenames to write and to allow the list of imports to be stored in a container rather than written to a file.
Intentionally minimal for now. Additional state translation will be added in future commits.
Replace the use of AddStream in the DTLTO ThinLTO backend to add files to the link with AddBuffer. Unlike the InProcess ThinLTO backend, DTLTO runs the backend compilation jobs by invoking an external process (currently clang). This writes the output object file to disk. Therefore, DTLTO requires a performant way of adding an existing file to the link. Note that the AddBuffer mechanism is also used for adding a file to the link if there is a cache hit.
Some of the code in LTO.h did not conform to the LLVM coding standard. However, it did match the style already used in that file. However this was causing automated code-formatting checks from Github to fail which was confusing on the PR. I decided to apply clang-format everywhere to prevent this - even though the new code no longer matches the style of the existing.
1. Use "with" when opening files. 2. Add module docstrings.
- Replace `thinlto-remote-opt-tool` with `thinlto-remote-compiler` - Make `thinlto-remote-compiler` a cl::opt
- Pull in documentation improvements.
I have changed this to be `PID`. Note that I expect the naming strategy to change in the future as we get feedback from more distribution systems. I have some concerns that mixing the PID into the filenames might prevent the caching systems for some distribution systems from working effectively.
Thanks. Fixed. > Redundant with earlier line Thanks. Remove the first check leaving only the -save-temps one. > Similar to above suggest s/INDEX/IMPORT/ Thanks. Fixed.
No. Thanks. Fixed now.
…nt string be used here? Thanks. Nice comment. I have removed all the tracing code from the new backend now. I proposed to add time tracing in a separate later PR. Is that acceptable?
Apologies! Something left over from an older revision. I have fixed it to read "(optionally) imports to disk".
…lang and lld patches go in? If so, do we need cross project testing right now? Wondering if it would be better to just stick with the testing you have in this patch that is under llvm/test, and add the cross project testing using only external tools once those patches are in. This test checks that the DTLTO code that the code that creates Clang command lines generates the expected options and that those options are accepted by clang. I wrote it to use llvm-lto and opt so it could be part of this commit before the clang and lld patches go in. However, I think it is a nice principle to only use external tools in cross-project-tests. I have removed the test from this PR with the intention of adding it back later. Additionally if we can use your idea (llvm#127749 (comment)) of storing the Clang `-c` step options and applying them to the remote backend compilations then this test will be rewritten to test that behaviour instead.
bd1976bris
force-pushed
the
DTLTO_llvm_only
branch
from
142bd00 to
8b109cb
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.