Page MenuHomePhabricator

[LTO][ELF] Add selective --save-temps= option
ClosedPublic

Authored by Northbadge on Jun 14 2022, 1:00 PM.

Details

Summary

Allows specific “temps” to be saved, instead of the current all-or-nothing nature of --save-temps. Multiple of these “temps” can be saved by specifying the argument multiple times.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
MaskRay added inline comments.Jun 14 2022, 10:55 PM
lld/test/ELF/lto/save-temps-eq.ll
3

Using glob with rm is dangerous.

14

You can use ls with glob. find in lit is very unusual.

101

use a normal x86_64 triple.

llvm/lib/LTO/LTOBackend.cpp
130
Northbadge marked 5 inline comments as done.

Resolved inline comments. Thanks for the review!

tejohnson added inline comments.Jun 21 2022, 7:18 PM
lld/test/ELF/lto/save-temps-eq.ll
20

Probably want to test to ensure which temp files were emitted by each of these? Edit: I do see now how you are removing them one by one and check for an empty %t.all at the end, but I think it would be clearer to check which file is emitted more directly.

Also, can you test that the options are composable? I.e. that you can specify multiple and get the union of the requested temps files?

99

It's really hard to follow what the checking below is doing, with the various for loops and diffs. Can the output file checks be made explicit, similar to what I suggested above?

Northbadge marked 2 inline comments as done.

Reworked the test case following feedback, thanks!

From a layering perspective, the feature is better tested on the llvm/ project, not lld/. You may need to change llvm-lto2 to check the values. Having tests in lld/test/ELF is fine too as that reflects how people use the functionality, but they should not be replacement for llvm/ side tests.
(llvm/test/ThinLTO/X86 and llvm/test/LTO/X86)

MaskRay added inline comments.Jun 22 2022, 5:16 PM
lld/ELF/Driver.cpp
1160
1161

Since there are only very few allowed values, SmallVector<StringRef, 0> may work better.
You may deduplicate the values in lld/ELF/Driver.cpp

How about this: remove bool config->saveTemps. If --save-temps is specified, populate the array with all allowed values.

1162

The comment is unnecessary.

lld/test/ELF/lto/save-temps-eq.ll
10

With mkdir %t && cd %t, all other %t references can be removed.
If you want to test a subdirectory behavior, create a directory without the %t prefix.

Northbadge added inline comments.Jun 22 2022, 6:15 PM
lld/ELF/Driver.cpp
1161

Wouldn't deduplicating the vector necessitate using a set (or sorting)? Also, if it were switched to being a vector there'd need to be a loop to check whether to save temps at a given stage, specifically at "prelink" (I placed a self-executing lambda there earlier on, it's not great for readability)

I agree with removing the bool though

Northbadge marked 3 inline comments as done.

Removed the saveTemps bool, updated test to use subdirectories

From a layering perspective, the feature is better tested on the llvm/ project, not lld/. You may need to change llvm-lto2 to check the values. Having tests in lld/test/ELF is fine too as that reflects how people use the functionality, but they should not be replacement for llvm/ side tests.
(llvm/test/ThinLTO/X86 and llvm/test/LTO/X86)

This is a good point, and for D127779 as well.

Northbadge added a comment.EditedJun 24 2022, 4:48 PM

From a layering perspective, the feature is better tested on the llvm/ project, not lld/. You may need to change llvm-lto2 to check the values. Having tests in lld/test/ELF is fine too as that reflects how people use the functionality, but they should not be replacement for llvm/ side tests.
(llvm/test/ThinLTO/X86 and llvm/test/LTO/X86)

This is a good point, and for D127779 as well.

I defer to the both of you on that point. However, this patch and D127779 are more of an extension of --save-temps (in my opinion), and as such the tests are predicated on --save-temps being correct. I just assumed --save-temps was properly tested to begin with, but it seems to have only been tested within ELF/lto on a basic level. Would it be sufficient (for these 2 patches) to write llvm side tests for --save-temps as a separate patch?

From a layering perspective, the feature is better tested on the llvm/ project, not lld/. You may need to change llvm-lto2 to check the values. Having tests in lld/test/ELF is fine too as that reflects how people use the functionality, but they should not be replacement for llvm/ side tests.
(llvm/test/ThinLTO/X86 and llvm/test/LTO/X86)

This is a good point, and for D127779 as well.

I defer to the both of you on that point. However, this patch and D127779 are more of an extension of --save-temps (in my opinion), and as such the tests are predicated on --save-temps being correct. I just assumed --save-temps was properly tested to begin with, but it seems to have only been tested within ELF/lto on a basic level. Would it be sufficient (for these 2 patches) to write llvm side tests for --save-temps as a separate patch?

It's not the --save-temps option that should be tested by llvm-lto2, but rather the core LTO changes (which are linker independent) being introduced in the patches. Typically when we add functionality to LTO we test it independently of any linker, via llvm-lto2, in one or both of the test directories @MaskRay mentioned above, then also add tests for whatever linker is utilizing the LTO support.

I missed this in D127777, which should ideally also have some llvm-lto2 based testing. E.g. see the -thinlto-distributed-indexes option in llvm/tools/llvm-lto2/llvm-lto2.cpp which is testing the thinlto-index-only functionality (llvm-lto2.cpp also has a -save-temps option). There are a bunch of tests under llvm/test/ThinLTO/X86/ that utilize these options (and end up testing the related LTO functionality in the process).

From a layering perspective, the feature is better tested on the llvm/ project, not lld/. You may need to change llvm-lto2 to check the values. Having tests in lld/test/ELF is fine too as that reflects how people use the functionality, but they should not be replacement for llvm/ side tests.
(llvm/test/ThinLTO/X86 and llvm/test/LTO/X86)

This is a good point, and for D127779 as well.

I defer to the both of you on that point. However, this patch and D127779 are more of an extension of --save-temps (in my opinion), and as such the tests are predicated on --save-temps being correct. I just assumed --save-temps was properly tested to begin with, but it seems to have only been tested within ELF/lto on a basic level. Would it be sufficient (for these 2 patches) to write llvm side tests for --save-temps as a separate patch?

It's not the --save-temps option that should be tested by llvm-lto2, but rather the core LTO changes (which are linker independent) being introduced in the patches. Typically when we add functionality to LTO we test it independently of any linker, via llvm-lto2, in one or both of the test directories @MaskRay mentioned above, then also add tests for whatever linker is utilizing the LTO support.

I missed this in D127777, which should ideally also have some llvm-lto2 based testing. E.g. see the -thinlto-distributed-indexes option in llvm/tools/llvm-lto2/llvm-lto2.cpp which is testing the thinlto-index-only functionality (llvm-lto2.cpp also has a -save-temps option). There are a bunch of tests under llvm/test/ThinLTO/X86/ that utilize these options (and end up testing the related LTO functionality in the process).

Ah ok, thanks for clarifying that! I'll get on those right away along with a separate patch for D127777 's missing tests

Ping on the llvm-lto2 change.

lld/ELF/Config.h
219

Use llvm::DenseSet<StringRef>.

SmallSet performs worse due to std::set.

lld/ELF/Driver.cpp
1161

Add static const char *saveTempsValues[] = {...};

Then append the values in if (args.hasArg(OPT_save_temps)) {

Below change s == "resolution" || s == "preopt" || s == "postpromote" to llvm::is_contained(saveTempsValues, ...)

llvm/include/llvm/LTO/Config.h
18

remove

275

Use llvm::DenseSet<StringRef>.

llvm/lib/LTO/LTOBackend.cpp
152

Shall we omit post from postinternalize,postimport,postopt so that the names match the temporary file names?

Added LLVM-side tests, modified llvm-lto2, fixed some capitalization and typos in tests.

Ping on the llvm-lto2 change.

Oh, didn't see this, was prepping the patch, i'll get one the comments now

Northbadge marked 6 inline comments as done.

Removed post* prefix, use DenseSet<StringRef> to store args

lld/ELF/Driver.cpp
1161

Thanks! didn't know about those. I changed it to use llvm::find since I needed the static string's address, not sure if there's a better way

MaskRay added inline comments.Jun 29 2022, 7:05 PM
lld/ELF/Driver.cpp
1164

Use args.filtered(OPT_save_temps_eq) which avoids a temporary std::vector. Then you can just use the value since its backing memory has sufficient lifetime. And you can avoid adl_end below.

1166

std::end

1169
lld/test/ELF/lto/save-temps-eq.ll
20

Prefer cmp for binary comparison.

llvm/include/llvm/LTO/Config.h
275

Omit llvm:: prefix since the delcaration is in namespace llvm { ... }

llvm/lib/LTO/LTOBackend.cpp
86

omit llvm:: prefix

llvm/test/ThinLTO/X86/selective-save-temps.ll
6

If you use ;; for non-RUN non-CHECK lines, change this ; too. Be consistent.

15

-r per line may be too verbose. You can pack multiple -r on one line.

llvm/tools/llvm-lto2/llvm-lto2.cpp
75

selected

Northbadge marked 9 inline comments as done.

Update to use DenseSet, cleaned up tests and arg parsing. Thanks for the feedback!

MaskRay accepted this revision.Jun 30 2022, 11:45 AM

\opt caused a Windows failure: https://buildkite.com/llvm-project/premerge-checks/builds/100428#0181b59c-9093-413f-bee2-39f2a6364181

Use --save-temps=opt

POSIX.1-2008 says:

A <backslash> that is not quoted shall preserve the literal value of the following character, with the exception of a <newline>.

therefore \opt is interpreted as opt. But Windows doesn't seem to do this.

Please fix them.

lld/ELF/Driver.cpp
1161
lld/test/ELF/lto/save-temps-eq.ll
19

\opt caused a Windows failure: https://buildkite.com/llvm-project/premerge-checks/builds/100428#0181b59c-9093-413f-bee2-39f2a6364181

--save-temps=opt

POSIX.1-2008 says:

A <backslash> that is not quoted shall preserve the literal value of the following character, with the exception of a <newline>.

therefore \opt is interpreted as opt. But Windows doesn't seem to do this.

61
llvm/test/ThinLTO/X86/selective-save-temps.ll
2

This comment may be inverse (due to layering). The lld/ELF test can say that it is similar to this file.

llvm/tools/llvm-lto2/llvm-lto2.cpp
277

remove else since the early return pattern is used.

283
This revision is now accepted and ready to land.Jun 30 2022, 11:45 AM
Northbadge added inline comments.Jun 30 2022, 12:56 PM
lld/test/ELF/lto/save-temps-eq.ll
19

Aha, was running into an issue with plain opt being translated/preprocessed to the full path of opt (the executable) , so it called --save-temps=/path/to/opt. Will see what I can do about it

Northbadge marked 6 inline comments as done.

Resolved inline comments; Also having buildbots test a potential fix for escaping characters on Windows (will split it into a separate patch if it works)

MaskRay requested changes to this revision.Jul 1 2022, 11:52 AM
MaskRay added inline comments.
llvm/utils/lit/lit/TestRunner.py
1163 ↗(On Diff #441760)

Don't introduce this unrelated change in this patch.

This revision now requires changes to proceed.Jul 1 2022, 11:52 AM
MaskRay added inline comments.Jul 1 2022, 11:53 AM
lld/test/ELF/lto/save-temps-eq.ll
61

I am confused. What does this %\opt do?

MaskRay accepted this revision.Jul 1 2022, 11:54 AM
MaskRay added inline comments.
lld/test/ELF/lto/save-temps-eq.ll
61

Ah, ok. If this is to avoid lit substitution, does 'opt' work?

This revision is now accepted and ready to land.Jul 1 2022, 11:54 AM
Northbadge added inline comments.Jul 1 2022, 11:54 AM
lld/test/ELF/lto/save-temps-eq.ll
61

Nope

llvm/utils/lit/lit/TestRunner.py
1163 ↗(On Diff #441760)

Yep, got that! I don't have access to a windows machine to test so I'm having the buildbots do it before splitting it off to a separate patch

Northbadge updated this revision to Diff 441790.Jul 1 2022, 3:04 PM
Northbadge marked 3 inline comments as done.

Walked back the lit change for cross-platform escape char (didn't work). Disabled the tests on Windows instead, let me know if I should avoid doing that. I'll probably land this after the long weekend. Thanks for all the feedback!

MaskRay accepted this revision.Jul 1 2022, 3:46 PM

Unsupporting Windows is fine. I am surprised that lit substitution happens even with xxx='opt' and xxx="opt". I'd expect that it only triggers for xxx opt yyy.

lld/test/ELF/lto/save-temps-eq.ll
5
Northbadge updated this revision to Diff 441803.Jul 1 2022, 3:52 PM

Missed semicolons.

MaskRay accepted this revision.Jul 4 2022, 10:10 PM
This revision was landed with ongoing or failed builds.Jul 6 2022, 10:07 AM
This revision was automatically updated to reflect the committed changes.
Northbadge marked an inline comment as done.