[ThinLTO] Migrate ThinLTOBitcodeWriter to the new PM.

[timshen]

Summary

Also see D33429 for other ThinLTO + New PM related changes.

[mehdi_amini]

llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp

423 ↗

(On Diff #100183)

Can't you do it with a lambda?

[timshen]

llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp

423 ↗

(On Diff #100183)

I can, except that AAR needs to be allocated outside of the lambda, on the stack:

Optional<AAResults> AAR;
auto F = [&AAR](Function &) -> AAResults& { ... };

(AAR can't be captured by value, since AAResults is not copyable) this way AAR out-lives the lambda, and I feel less readable.

[chandlerc]

(focusing on the LLVM side of this review for now)

Can you add an LLVM-based test? Can you add this to lib/Passes/PassRegistry.def?

llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp

423 ↗

(On Diff #100183)

You don't need to keep a copy though... You can directly expose the reference returned from the FunctionAnalysisManager -- that reference will stay alive until the result is invalidated. So this will become something more like:

[&FAM](Function &F) { return FAM.getResult<AAManager>(F); }

[timshen]

In D33525#764251, @chandlerc wrote:

(focusing on the LLVM side of this review for now)

Can you add an LLVM-based test? Can you add this to lib/Passes/PassRegistry.def?

I see that not all passes are registered (BitcodeWriterPass). Is it an oversight?

Since ThinkLTOBitcodeWriter takes a special raw_ostream to construct, I may end up with changing PassBuilder.cpp to make it registered.

[pcc]

clang/test/CodeGen/thin_link_bitcode.c

5 ↗

(On Diff #100183)

s/NEW_PM/NO_DEBUG/ and remove line 12.

I think you also want to test the contents of %t here.

[timshen]

Change the test case.

[timshen]

In D33525#764251, @chandlerc wrote:

(focusing on the LLVM side of this review for now)

Can you add an LLVM-based test? Can you add this to lib/Passes/PassRegistry.def?

Talked offline. Given the fact that BitcodeWriter (and possibly assembly writer?) is not registered either, it seems to be a larger issue that's out of the scope of this patch.

[chandlerc]

In D33525#766050, @timshen wrote:

In D33525#764251, @chandlerc wrote:

(focusing on the LLVM side of this review for now)

Can you add an LLVM-based test? Can you add this to lib/Passes/PassRegistry.def?

Talked offline. Given the fact that BitcodeWriter (and possibly assembly writer?) is not registered either, it seems to be a larger issue that's out of the scope of this patch.

While *generic* testing is out of scope, I think you need at least *some* testing of this in this patch. It can be an option directly in the llvm-lto tool or a direct option in the opt tool itself, but we shouldn't add a bunch of code to LLVM that can only ever be exercised by using some other tool.

[timshen]

Add opt support and llvm test.

[chandlerc]

clang/lib/CodeGen/BackendUtil.cpp

913–914 ↗

(On Diff #100488)

The clang side of this should probably be a separate review on cfe-commits, but one drive-by coment.

I find this usage of emplace much harder to read than:

ThinLinkOS.reset(raw_fd_ostream(CodeGenOpts.ThinLinkBitcodeFile, EC,
                                llvm::sys::fs::F_None));

Unless we just can't do the above for some reason (it won't compile =[) I would prefer it.

llvm/include/llvm/Transforms/IPO/ThinLTOBitcodeWriter.h

31–32 ↗

(On Diff #100488)

Move (or add) a comment about this API to thea header? Notably, under what circumstances can ThinLinkOS be null? What happens if it is?

llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp

9–12 ↗

(On Diff #100488)

Can just omit the added section I suspect.

443 ↗

(On Diff #100488)

Are we unable to deduce the return type here?

llvm/test/Transforms/ThinLTOBitcodeWriter/new-pm.ll

1 ↗

(On Diff #100488)

Why run any passes here?

llvm/tools/opt/NewPMDriver.h

61 ↗

(On Diff #100488)

Why an optional pointer? Maybe just allow the pointer to be null if there isn't a thin link output file?

llvm/tools/opt/opt.cpp

534 ↗

(On Diff #100488)

Maybe unnecessary based on the above comment, but generally I would just assign rather than using emplace.

541–542 ↗

(On Diff #100488)

We do Out->keep() in runPassPipeline, why not do this there as well?

[timshen]

llvm/test/Transforms/ThinLTOBitcodeWriter/new-pm.ll

1 ↗

(On Diff #100488)

I saw that the runPassPipeline call is in if (PassPipeline.getNumOccurrences() > 0) {. I take that as "must specify -passes" in order to run the new pass manager (which is created in runPassPipeline). And specifiy an empty -passes cause an error message "unable to parse pass pipeline description".

llvm/tools/opt/NewPMDriver.h

61 ↗

(On Diff #100488)

There are 3 states:

1. Use ThinLTOBitcodeWrite pass, controlled by OutputThinLTOBC, and the output file is not null.
2. OutputThinLTOBC is true, but output file is null (don't write to the file).
3. OutputThinLTOBC is false.

An optional nullable pointer, confusing as it might be (but I'm not sure how much), provides 3 states.

If we instead pass in a bool and a pointer, that's 4 states, one of which is invalid.

[chandlerc]

llvm/test/Transforms/ThinLTOBitcodeWriter/new-pm.ll

1 ↗

(On Diff #100488)

You can use '-passes=no-op-module' or something similar to at least minimize how much occurs using the new PM?

llvm/tools/opt/NewPMDriver.h

61 ↗

(On Diff #100488)

Ok, but is #2 in your list actually useful? Maybe it is, but if it is then at the very least the comment should make it super clear what is going on here. Otherwise, I suspect many readers will assume than when the optional is engaged the pointer isn't null as that's just too "obvious".

[timshen]

Updated based on the comments, and left out Clang changes for a separate patch.

llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp

443 ↗

(On Diff #100488)

No, because return type deduction uses "auto deduction" instead of "decltype deduction" - it deduces to a value type.

llvm/tools/opt/NewPMDriver.h

61 ↗

(On Diff #100488)

#2 is consistent with opt's legacy pass manager behavior (search "createWriteThinLTOBitcodePass" in opt.cpp).

I added an emphasized "*nullable*" in the comment. :)

[timshen]

Rebase off from D33429. I realized that they are independent changes to LLVM.

[chandlerc]

Still not really happy with htis API, but let's fix it in a follow-up as it will grow the scope and much of the problem was already there (and the API is fairly narrow in scope).

llvm/tools/opt/NewPMDriver.h

61 ↗

(On Diff #100488)

Ok, but this API is really, really confusing.

We should just add a new output kind to select between normal bitcode and thinlto bitcode. And then this can just be a pointer indicating if we want an additional thin link output.

But I'm happy to make that change in a follow-up (or for you to) as it'll need to change both PMs.

llvm/tools/opt/opt.cpp

532–534 ↗

(On Diff #100911)

Inline this into the call?

OutputThinLTOBC ? ThinLinkOut.get : None

(maybe with {} around it...)

[chandlerc]

Sorry, previous comment should have said LGTM with the tweak suggested.

[timshen]

Add ThinLTOBitcode as a new OutputKind to opt.

[timshen]

llvm/tools/opt/NewPMDriver.h

61 ↗

(On Diff #100488)

Done in the same patch. It's actually not that many changes to add an OutputKind for the new PM in opt. The old PM in opt doesn't even use such an enum (that's probably why they have these booleans :).

llvm/tools/opt/opt.cpp

532–534 ↗

(On Diff #100911)

It doesn't compile, because the two values provided by ?: should have the same type. :)

Also the Optional is gone. :)

[timshen]

Keep the link file only when in ThinLTO bitcode mode.

[chandlerc]

LGTM, much nicer API now thanks!

llvm/trunk/include/llvm/Transforms/IPO/ThinLTOBitcodeWriter.h

+//===- ThinLTOBitcodeWriter.h - Bitcode writing pass for ThinLTO ----------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+//
+// This pass prepares a module containing type metadata for ThinLTO by splitting
+// it into regular and thin LTO parts if possible, and writing both parts to
+// a multi-module bitcode file. Modules that do not contain type metadata are
+// written unmodified as a single module.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_TRANSFORMS_IPO_THINLTOBITCODEWRITER_H
+#define LLVM_TRANSFORMS_IPO_THINLTOBITCODEWRITER_H
+
+#include <llvm/IR/PassManager.h>
+#include <llvm/Support/raw_ostream.h>
+
+namespace llvm {
+
+class ThinLTOBitcodeWriterPass
+    : public PassInfoMixin<ThinLTOBitcodeWriterPass> {
+  raw_ostream &OS;
+  raw_ostream *ThinLinkOS;
+
+public:
+  // Writes bitcode to OS. Also write thin link file to ThinLinkOS, if
+  // it's not nullptr.
+  ThinLTOBitcodeWriterPass(raw_ostream &OS, raw_ostream *ThinLinkOS)
+      : OS(OS), ThinLinkOS(ThinLinkOS) {}
+
+  PreservedAnalyses run(Module &M, ModuleAnalysisManager &AM);
+};
+
+} // namespace llvm
+
+#endif

llvm/trunk/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp

 //===- ThinLTOBitcodeWriter.cpp - Bitcode writing pass for ThinLTO --------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
-//
-// This pass prepares a module containing type metadata for ThinLTO by splitting
-// it into regular and thin LTO parts if possible, and writing both parts to
-// a multi-module bitcode file. Modules that do not contain type metadata are
-// written unmodified as a single module.
-//
-//===----------------------------------------------------------------------===//
 
+#include "llvm/Transforms/IPO/ThinLTOBitcodeWriter.h"
 #include "llvm/Analysis/BasicAliasAnalysis.h"
 #include "llvm/Analysis/ModuleSummaryAnalysis.h"
 #include "llvm/Analysis/ProfileSummaryInfo.h"
 #include "llvm/Analysis/TypeMetadataUtils.h"
 #include "llvm/Bitcode/BitcodeWriter.h"
 #include "llvm/IR/Constants.h"
 #include "llvm/IR/DebugInfo.h"
 #include "llvm/IR/Intrinsics.h"

 INITIALIZE_PASS_DEPENDENCY(TargetLibraryInfoWrapperPass)
 INITIALIZE_PASS_END(WriteThinLTOBitcode, "write-thinlto-bitcode",
                     "Write ThinLTO Bitcode", false, true)
 
 ModulePass *llvm::createWriteThinLTOBitcodePass(raw_ostream &Str,
                                                 raw_ostream *ThinLinkOS) {
   return new WriteThinLTOBitcode(Str, ThinLinkOS);
 }
+
+PreservedAnalyses
+llvm::ThinLTOBitcodeWriterPass::run(Module &M, ModuleAnalysisManager &AM) {
+  FunctionAnalysisManager &FAM =
+      AM.getResult<FunctionAnalysisManagerModuleProxy>(M).getManager();
+  writeThinLTOBitcode(OS, ThinLinkOS,
+                      [&FAM](Function &F) -> AAResults & {
+                        return FAM.getResult<AAManager>(F);
+                      },
+                      M, &AM.getResult<ModuleSummaryIndexAnalysis>(M));
+  return PreservedAnalyses::all();
+}

llvm/trunk/test/Transforms/ThinLTOBitcodeWriter/new-pm.ll

+; RUN: opt -passes='no-op-module' -debug-pass-manager -thinlto-bc -thin-link-bitcode-file=%t2 -o %t %s 2>&1 | FileCheck %s --check-prefix=DEBUG_PM
+; RUN: llvm-bcanalyzer -dump %t2 | FileCheck %s --check-prefix=BITCODE
+
+; DEBUG_PM: ThinLTOBitcodeWriterPass
+; BITCODE: Foo
+
+define void @Foo() {
+  ret void
+}

llvm/trunk/tools/opt/NewPMDriver.h

 class Module;
 class TargetMachine;
 class tool_output_file;
 
 namespace opt_tool {
 enum OutputKind {
   OK_NoOutput,
   OK_OutputAssembly,
-  OK_OutputBitcode
+  OK_OutputBitcode,
+  OK_OutputThinLTOBitcode,
 };
 enum VerifierKind {
   VK_NoVerifier,
   VK_VerifyInAndOut,
   VK_VerifyEachPass
 };
 }
 
 /// \brief Driver function to run the new pass manager over a module.
 ///
 /// This function only exists factored away from opt.cpp in order to prevent
 /// inclusion of the new pass manager headers and the old headers into the same
 /// file. It's interface is consequentially somewhat ad-hoc, but will go away
 /// when the transition finishes.
-bool runPassPipeline(StringRef Arg0, Module &M,
-                     TargetMachine *TM, tool_output_file *Out,
+///
+/// ThinLTOLinkOut is only used when OK is OK_OutputThinLTOBitcode, and can be
+/// nullptr.
+bool runPassPipeline(StringRef Arg0, Module &M, TargetMachine *TM,
+                     tool_output_file *Out, tool_output_file *ThinLinkOut,
                      StringRef PassPipeline, opt_tool::OutputKind OK,
                      opt_tool::VerifierKind VK,
                      bool ShouldPreserveAssemblyUseListOrder,
                      bool ShouldPreserveBitcodeUseListOrder,
                      bool EmitSummaryIndex, bool EmitModuleHash);
 }
 
 #endif

llvm/trunk/tools/opt/NewPMDriver.cpp

 #include "llvm/IR/Module.h"
 #include "llvm/IR/PassManager.h"
 #include "llvm/IR/Verifier.h"
 #include "llvm/Passes/PassBuilder.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/ToolOutputFile.h"
 #include "llvm/Target/TargetMachine.h"
+#include "llvm/Transforms/IPO/ThinLTOBitcodeWriter.h"
 #include "llvm/Transforms/Scalar/LoopPassManager.h"
 
 using namespace llvm;
 using namespace opt_tool;
 
 static cl::opt<bool>
     DebugPM("debug-pass-manager", cl::Hidden,
             cl::desc("Print pass management debugging information"));
 
 // This flag specifies a textual description of the alias analysis pipeline to
 // use when querying for aliasing information. It only works in concert with
 // the "passes" flag above.
 static cl::opt<std::string>
     AAPipeline("aa-pipeline",
                cl::desc("A textual description of the alias analysis "
                         "pipeline for handling managed aliasing queries"),
                cl::Hidden);
 
-bool llvm::runPassPipeline(StringRef Arg0, Module &M,
-                           TargetMachine *TM, tool_output_file *Out,
+bool llvm::runPassPipeline(StringRef Arg0, Module &M, TargetMachine *TM,
+                           tool_output_file *Out,
+                           tool_output_file *ThinLTOLinkOut,
                            StringRef PassPipeline, OutputKind OK,
                            VerifierKind VK,
                            bool ShouldPreserveAssemblyUseListOrder,
                            bool ShouldPreserveBitcodeUseListOrder,
                            bool EmitSummaryIndex, bool EmitModuleHash) {
   PassBuilder PB(TM);
 
   // Specially handle the alias analysis manager so that we can register
bool llvm::runPassPipeline(StringRef Arg0, Module &M, TargetMachine *TM,
   case OK_OutputAssembly:
     MPM.addPass(
         PrintModulePass(Out->os(), "", ShouldPreserveAssemblyUseListOrder));
     break;
   case OK_OutputBitcode:
     MPM.addPass(BitcodeWriterPass(Out->os(), ShouldPreserveBitcodeUseListOrder,
                                   EmitSummaryIndex, EmitModuleHash));
     break;
+  case OK_OutputThinLTOBitcode:
+    MPM.addPass(ThinLTOBitcodeWriterPass(
+        Out->os(), ThinLTOLinkOut ? &ThinLTOLinkOut->os() : nullptr));
+    break;
   }
 
   // Before executing passes, print the final values of the LLVM options.
   cl::PrintOptionValues();
 
   // Now that we have all of the passes ready, run them.
   MPM.run(M, MAM);
 
   // Declare success.
-  if (OK != OK_NoOutput)
+  if (OK != OK_NoOutput) {
     Out->keep();
+    if (OK == OK_OutputThinLTOBitcode && ThinLTOLinkOut)
+      ThinLTOLinkOut->keep();
+  }
   return true;
 }

llvm/trunk/tools/opt/opt.cpp

#endif
   // impress anyone by spewing tons of binary goo to a terminal.
   if (!Force && !NoOutput && !AnalyzeOnly && !OutputAssembly)
     if (CheckBitcodeOutputToConsole(Out->os(), !Quiet))
       NoOutput = true;
 
   if (PassPipeline.getNumOccurrences() > 0) {
     OutputKind OK = OK_NoOutput;
     if (!NoOutput)
-      OK = OutputAssembly ? OK_OutputAssembly : OK_OutputBitcode;
+      OK = OutputAssembly
+               ? OK_OutputAssembly
+               : (OutputThinLTOBC ? OK_OutputThinLTOBitcode : OK_OutputBitcode);
 
     VerifierKind VK = VK_VerifyInAndOut;
     if (NoVerify)
       VK = VK_NoVerifier;
     else if (VerifyEach)
       VK = VK_VerifyEachPass;
 
     // The user has asked to use the new pass manager and provided a pipeline
     // string. Hand off the rest of the functionality to the new code for that
     // layer.
-    return runPassPipeline(argv[0], *M, TM.get(), Out.get(),
+    return runPassPipeline(argv[0], *M, TM.get(), Out.get(), ThinLinkOut.get(),
                            PassPipeline, OK, VK, PreserveAssemblyUseListOrder,
                            PreserveBitcodeUseListOrder, EmitSummaryIndex,
                            EmitModuleHash)
                ? 0
                : 1;
   }
 
   // Create a PassManager to hold and optimize the collection of passes we are