[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

Can't you do it with a lambda?

[timshen]

llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp

423

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

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

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

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

32–33

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

Can just omit the added section I suspect.

458

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

458

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!

clang/lib/CodeGen/BackendUtil.cpp

 #include "llvm/Support/raw_ostream.h"
 #include "llvm/Target/TargetMachine.h"
 #include "llvm/Target/TargetOptions.h"
 #include "llvm/Target/TargetSubtargetInfo.h"
 #include "llvm/Transforms/Coroutines.h"
 #include "llvm/Transforms/IPO.h"
 #include "llvm/Transforms/IPO/AlwaysInliner.h"
 #include "llvm/Transforms/IPO/PassManagerBuilder.h"
+#include "llvm/Transforms/IPO/ThinLTOBitcodeWriter.h"
 #include "llvm/Transforms/Instrumentation.h"
 #include "llvm/Transforms/ObjCARC.h"
 #include "llvm/Transforms/Scalar.h"
 #include "llvm/Transforms/Scalar/GVN.h"
 #include "llvm/Transforms/Utils/SymbolRewriter.h"
 #include <memory>
 using namespace clang;
 using namespace llvm;
if (CodeGenOpts.OptimizationLevel == 0) {
       MPM = PB.buildPerModuleDefaultPipeline(Level);
     }
   }
 
   // FIXME: We still use the legacy pass manager to do code generation. We
   // create that pass manager here and use it as needed below.
   legacy::PassManager CodeGenPasses;
   bool NeedCodeGen = false;
+  Optional<raw_fd_ostream> ThinLinkOS;
 
   // Append any output we need to the pass manager.
   switch (Action) {
   case Backend_EmitNothing:
     break;
 
   case Backend_EmitBC:
+
+    if (CodeGenOpts.EmitSummaryIndex) {
+      if (!CodeGenOpts.ThinLinkBitcodeFile.empty()) {
+        std::error_code EC;
+        ThinLinkOS.emplace(CodeGenOpts.ThinLinkBitcodeFile, EC,
+                           llvm::sys::fs::F_None);
+        if (EC) {
+          Diags.Report(diag::err_fe_unable_to_open_output)
+              << CodeGenOpts.ThinLinkBitcodeFile << EC.message();
+          return;
+        }
+      }
+      MPM.addPass(
+          ThinLTOBitcodeWriterPass(*OS, ThinLinkOS ? &*ThinLinkOS : nullptr));
+    } else {
       MPM.addPass(BitcodeWriterPass(*OS, CodeGenOpts.EmitLLVMUseLists,
                                     CodeGenOpts.EmitSummaryIndex,
                                     CodeGenOpts.EmitSummaryIndex));
+    }
     break;
 
   case Backend_EmitLL:
     MPM.addPass(PrintModulePass(*OS, "", CodeGenOpts.EmitLLVMUseLists));
     break;
 
   case Backend_EmitAssembly:
   case Backend_EmitMCNull:

clang/test/CodeGen/thin_link_bitcode.c

 // RUN: %clang_cc1 -o %t -flto=thin -fthin-link-bitcode=%t.nodebug -triple x86_64-unknown-linux-gnu -emit-llvm-bc -debug-info-kind=limited %s
 // RUN: llvm-bcanalyzer -dump %t | FileCheck %s
 // RUN: llvm-bcanalyzer -dump %t.nodebug | FileCheck %s --check-prefix=NO_DEBUG
+// RUN: %clang_cc1 -o %t -flto=thin -fexperimental-new-pass-manager -fthin-link-bitcode=%t.newpm -triple x86_64-unknown-linux-gnu -emit-llvm-bc %s
+// RUN: llvm-bcanalyzer -dump %t.newpm | FileCheck %s --check-prefix=NEW_PM
 int main (void) {
   return 0;
 }
 
 // CHECK: COMPILE_UNIT
 // NO_DEBUG-NOT: COMPILE_UNIT
+// NEW_PM-NOT: COMPILE_UNIT

llvm/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:
+  ThinLTOBitcodeWriterPass(raw_ostream &OS, raw_ostream *ThinLinkOS)
+      : OS(OS), ThinLinkOS(ThinLinkOS) {}
+
+  PreservedAnalyses run(Module &M, ModuleAnalysisManager &AM);
+};
+
+} // namespace llvm
+
+#endif

llvm/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.
+// Pass implementation.
 //
 //===----------------------------------------------------------------------===//
 
+#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"
public:
   }
   void getAnalysisUsage(AnalysisUsage &AU) const override {
     AU.setPreservesAll();
     AU.addRequired<AssumptionCacheTracker>();
     AU.addRequired<ModuleSummaryIndexWrapperPass>();
     AU.addRequired<TargetLibraryInfoWrapperPass>();
   }
 };
+
+class AARGetter {
+  FunctionAnalysisManager &AM;
+  Optional<AAResults> AAR;
+
+public:
+  AARGetter(FunctionAnalysisManager &AM) : AM(AM) {}
+
+  AAResults &operator()(Function &F) {
+    if (!AAR)
+      AAR.emplace<AAResults>(AAManager().run(F, AM));
+    return *AAR;
+  }
+};
+
 } // anonymous namespace
 
 char WriteThinLTOBitcode::ID = 0;
 INITIALIZE_PASS_BEGIN(WriteThinLTOBitcode, "write-thinlto-bitcode",
                       "Write ThinLTO Bitcode", false, true)
 INITIALIZE_PASS_DEPENDENCY(AssumptionCacheTracker)
 INITIALIZE_PASS_DEPENDENCY(ModuleSummaryIndexWrapperPass)
 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) {
+  writeThinLTOBitcode(
+      OS, ThinLinkOS,
+      AARGetter(
+          AM.getResult<FunctionAnalysisManagerModuleProxy>(M).getManager()),
+      M, &AM.getResult<ModuleSummaryIndexAnalysis>(M));
+  return PreservedAnalyses::all();
+}