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.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
lld/ELF/Config.h | ||
---|---|---|
220 | use 0. this isn't a common option. having a large size makes accessing other bool variables slow. |
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 | Avoid a reserved identifier. https://en.cppreference.com/w/cpp/language/identifiers |
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? |
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)
lld/ELF/Driver.cpp | ||
---|---|---|
1160 | ||
1161 | Since there are only very few allowed values, SmallVector<StringRef, 0> may work better. 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. |
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 |
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.
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 |
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 |
\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 |
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 |
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)
llvm/utils/lit/lit/TestRunner.py | ||
---|---|---|
1163 ↗ | (On Diff #441760) | Don't introduce this unrelated change in this patch. |
lld/test/ELF/lto/save-temps-eq.ll | ||
---|---|---|
61 | I am confused. What does this %\opt do? |
lld/test/ELF/lto/save-temps-eq.ll | ||
---|---|---|
61 | Ah, ok. If this is to avoid lit substitution, does 'opt' work? |
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!
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 | ||
---|---|---|
6 |
Use llvm::DenseSet<StringRef>.
SmallSet performs worse due to std::set.