[libLTO] Add support for -save-temps.

[qiongsiwu1]

Summary

Update June 23, 2023: @fhahn is the original author of this patch. @qiongsiwu1 now continues the work.

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 LTOCodeGenerator's constructor. 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.

[fhahn]

(ld itself also supports -save-temps, but using the common infrastructure we can get more granular intermediary files, but at the moment not all hooks are set up in LTOCodeGenerator).

[steven_wu]

LTOCodegen can achieve most of the temp file by going through write_merged_module function. While I don't see this save-temps gains much advantage comparing the existing one except more granularity, it is also fine to have this new debug option. It would be good to switch the save-temp interface to using this flag in the future since it will simplify the code in libLTO and ld.

[steven_wu]

Can you add a test case? Otherwise, LGTM.

[tejohnson]

Should something equivalent be added to ThinLTOCodeGenerator.cpp?

[Whitney]

@fhahn Thanks for implementing it. When do you plan to commit this change?

[fhahn]

In D98183#2684205, @Whitney wrote:

@fhahn Thanks for implementing it. When do you plan to commit this change?

I still like to create a unit test for it. I'll commit it once I have time to do so.

[qiongsiwu1]

I updated this patch in https://reviews.llvm.org/D153559.

[tejohnson]

In D98183#2611454, @tejohnson wrote:

Should something equivalent be added to ThinLTOCodeGenerator.cpp?

I just noticed that ThinLTOCodeGenerator handles this differently, with the -thinlto-save-temps flag in llvm-lto.cpp - could that support be generalized to apply to both regular and Thin LTO in the legacy LTO API files and llvm-lto?

[qiongsiwu1]

I will continue @fhahn's work on this patch.

[qiongsiwu1]

Moving https://reviews.llvm.org/D153559 over to this patch. This update is simply a rebase. I will look into consolidating the options between thin and regular LTO's interface next.

[qiongsiwu1]

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

67 ↗

(On Diff #534062)

@tejohnson I am going back to this comment https://reviews.llvm.org/D153559#4441538 indicating that it may not be ideal to use the SaveTemps option defined in LTOCodeGenerator.cpp here for llvm-lto2. This particular change was made because we cannot have two save-temps coexisting. I thought about renaming one, or both options, but renaming could make both options confusing. Should I call the llvm-lto2 option lto2-save-temps? That is probably not ideal.

May I get some advice? Is it ok if we reuse the same option? Thanks in advance!

[tejohnson]

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

67 ↗

(On Diff #534062)

I wouldn't want to rename the existing flag in llvm-lto2, since a ton of tests already use it.

It looks like the convention in LTOCodeGenerator.cpp is to preface options with "lto-", so perhaps that one should be -lto-save-temps. That would also be more consistent with the -thinlto-save-temps option used for ThinLTOCodeGenerator.cpp (defined in llvm-lto.cpp).

While it could work to use the same option, it seems a bit odd to share in this case. I think it only works in this case because llvm-lto2 depends on the LTO library which currently includes the code for both LTO APIs, even though llvm-lto2 doesn't use the legacy one.

[qiongsiwu1]

I wouldn't want to rename the existing flag in llvm-lto2, since a ton of tests already use it.

It looks like the convention in LTOCodeGenerator.cpp is to preface options with "lto-", so perhaps that one should be -lto-save-temps. That would also be more consistent with the -thinlto-save-temps option used for ThinLTOCodeGenerator.cpp (defined in llvm-lto.cpp).

While it could work to use the same option, it seems a bit odd to share in this case. I think it only works in this case because llvm-lto2 depends on the LTO library which currently includes the code for both LTO APIs, even though llvm-lto2 doesn't use the legacy one.

@tejohnson Thanks so much for the suggestion! The new option is renamed to -lto-save-temps to avoid the conflict with llvm-lto2. I took a brief look to see if we can consolidate the option with thin LTO. My current understanding is that ThinLTOCodeGenerator and LTOCodeGenerator work quite differently so it may require further changes to consolidate the two. I am inclined to leave this patch in its current shape. That said, I don't have a lot of experience with thin LTO so I may over estimate the difficulty. Let me know if you think otherwise and I will take a closer look.

A test is added as well to test the LTOGenerator feature as it is.

[tejohnson]

llvm/lib/LTO/LTOCodeGenerator.cpp

633

It doesn't really make sense to use the post import hook for regular LTO, since that is a ThinLTO-specific optimization. The new LTO API uses the PreOptModuleHook, which is a better fit here (see LTO::runRegularLTO in LTO.cpp).

llvm/lib/LTO/LTOCodeGenerator.cpp

 
 cl::opt<bool>
     LTORunCSIRInstr("cs-profile-generate",
                     cl::desc("Perform context sensitive PGO instrumentation"));
 
 cl::opt<std::string>
     LTOCSIRProfile("cs-profile-path",
                    cl::desc("Context sensitive profile file path"));
+
+static cl::opt<bool> LTOSaveTemps(
+    "lto-save-temps", cl::init(false),
+    cl::desc("Save temporary LTO files in the current working directory."));
 } // namespace llvm
 
 LTOCodeGenerator::LTOCodeGenerator(LLVMContext &Context)
     : Context(Context), MergedModule(new Module("ld-temp.o", Context)),
       TheLinker(new Linker(*MergedModule)) {
   Context.setDiscardValueNames(LTODiscardValueNames);
   Context.enableDebugTypeODRUniquing();
 
   Config.CodeModel = std::nullopt;
   Config.StatsFile = LTOStatsFile;
   Config.PreCodeGenPassesHook = [](legacy::PassManager &PM) {
     PM.add(createObjCARCContractPass());
   };
 
   Config.RunCSIRInstr = LTORunCSIRInstr;
   Config.CSIRProfile = LTOCSIRProfile;
+
+  if (LTOSaveTemps)
+    if (Config.addSaveTemps("ld-temp.",
+                            /* UseInputModulePath */ true))
+      errs() << "Warning: failed to set up path to save temporary files\n";
 }
 
 LTOCodeGenerator::~LTOCodeGenerator() = default;
 
 void LTOCodeGenerator::setAsmUndefinedRefs(LTOModule *Mod) {
   for (const StringRef &Undef : Mod->getAsmUndefinedRefs())
     AsmUndefinedRefs.insert(Undef);
 }
bool LTOCodeGenerator::optimize() {
   verifyMergedModuleOnce();
 
   // Mark which symbols can not be internalized
   this->applyScopeRestrictions();
 
   // Add an appropriate DataLayout instance for this module...
   MergedModule->setDataLayout(TargetMach->createDataLayout());
 
+  if (Config.PostImportModuleHook)
+    Config.PostImportModuleHook(0, *MergedModule);
+
   if (!SaveIRBeforeOptPath.empty()) {
     std::error_code EC;
     raw_fd_ostream OS(SaveIRBeforeOptPath, EC, sys::fs::OF_None);
     if (EC)
       report_fatal_error(Twine("Failed to open ") + SaveIRBeforeOptPath +
                          " to save optimized bitcode\n");
     WriteBitcodeToFile(*MergedModule, OS,
                        /* ShouldPreserveUseListOrder */ true);

llvm/test/LTO/X86/lto-save-temps.ll

+; Check -lto-save-temps generates all the necessary temp files.
+; RUN: rm -rf %t.d
+; RUN: mkdir -p %t.d && cd %t.d
+; RUN: llvm-as %s -o %t1.bc
+; RUN: llvm-lto -lto-save-temps %t1.bc -o %t2.bc
+; RUN: llvm-dis ld-temp.0.3.import.bc -o - | FileCheck %s
+; RUN: llvm-dis ld-temp.0.4.opt.bc -o - | FileCheck %s
+; RUN: llvm-dis ld-temp.0.5.precodegen.bc -o - | FileCheck %s
+; RUN: ls ld-temp.resolution.txt
+
+; CHECK: source_filename = "ld-temp.o"
+
+define i32 @main() {
+entry:
+  %call = call i32 @foo()
+  ret i32 %call
+}
+
+declare i32 @foo()