This is an archive of the discontinued LLVM Phabricator instance.

[libLTO] Add support for -save-temps
AbandonedPublic

Authored by qiongsiwu1 on Jun 22 2023, 8:15 AM.

Details

Summary

This patch rebases https://reviews.llvm.org/D98183 and consolidates -save-temps across libLTO, llvm-lto and llvm-lto2.

Original summary of https://reviews.llvm.org/D98183:
This patch updates LTOCodeGenerator to use addSaveTemps to support the
-save-temps option when using libLTO. At the moment, the save-temps
hooks are set up in parseCodeGenDebugOptions, because that's the
earliest the options have been parsed. I am not sure if there's a better
place, given the way libLTO's API is structured. At the moment, the
temporary files are created in the current working directory, because I
do not think there's a way to get the path for the final binary
up-front.

It also adds support for running the PostImportModuleHook just before
IR optimiztions are run. At the moment, the following temporary files
are created:

ld-temp.0.3.import.bc (before IR optimizations)
ld-temp.0.4.opt.bc (after IR optimizations)
ld-temp.0.5.precodegen.bc (before codegen)

Other hooks can be supported in the future, but the 3 IR files above
should be the most valuable ones when investigating mis-compiles or
crashes when doing full LTO using libLTO.

Diff Detail

Event Timeline

qiongsiwu1 created this revision.Jun 22 2023, 8:15 AM
qiongsiwu1 requested review of this revision.Jun 22 2023, 8:15 AM
qiongsiwu1 added a comment.EditedJun 22 2023, 8:17 AM

@fhahn your original patch https://reviews.llvm.org/D98183 is very useful for us as well. I took your patch and rebased it. I tried updating the original patch but I did not find a way to do it since I am not the author. I think we have numerous existing tests for llvm-lto/llvm-lto2 -save-temps and they should be using the new option now. Let me know if this looks good! Thanks so much!

qiongsiwu1 edited the summary of this revision. (Show Details)Jun 22 2023, 8:18 AM

It would be better to work with @fhahn on the old patch (you can commandeer it if he is ok with that), so that comment history is preserved. But I have some comments on this version that I will add here. Looks like a test is still needed (this patch affects legacy regular LTO, the prior llvm-lto tests would have only used the existing option for ThinLTO - see thinlto-save-temps flag in llvm-lto.cpp). I'll leave a comment on the original version, I think it makes sense to common up the options used for this between thin and regular LTO in the legacy LTO API and its tool, llvm-lto.

llvm/tools/llvm-lto2/llvm-lto2.cpp
67

llvm-lto2.cpp tests the new LTO API, so it is a bit odd for it to use a flag from the legacy LTO API.

I'm not sure it makes sense to common the flag between these 2 different interfaces and their corresponding tools.

llvm/tools/lto/lto.cpp
36

Unused in this file

It would be better to work with @fhahn on the old patch (you can commandeer it if he is ok with that), so that comment history is preserved. But I have some comments on this version that I will add here. Looks like a test is still needed (this patch affects legacy regular LTO, the prior llvm-lto tests would have only used the existing option for ThinLTO - see thinlto-save-temps flag in llvm-lto.cpp). I'll leave a comment on the original version, I think it makes sense to common up the options used for this between thin and regular LTO in the legacy LTO API and its tool, llvm-lto.

Thanks so much for the comment @tejohnson !! @fhahn Let me know if you are ok with me taking over the original patch https://reviews.llvm.org/D98183. I will close this patch and continue the work there (consolidating options between LTO/thinLTO and adding tests). Thanks!

fhahn added a comment.Jun 22 2023, 2:19 PM

@qiongsiwu1 please feel free to commandeer the patch. Would be good to update the description to make this clear.