[Remarks] Restructure bitstream remarks to be fully standalone (#156715)

Currently there are two serialization modes for bitstream Remarks:
standalone and separate. The separate mode splits remark metadata (e.g.
the string table) from actual remark data. The metadata is written into
the object file by the AsmPrinter, while the remark data is stored in a
separate remarks file. This means we can't use bitstream remarks with
tools like opt that don't generate an object file. Also, it is confusing
to post-process bitstream remarks files, because only the standalone
files can be read by llvm-remarkutil. We always need to use dsymutil
to convert the separate files to standalone files, which only works for
MachO. It is not possible for clang/opt to directly emit bitstream
remark files in standalone mode, because the string table can only be
serialized after all remarks were emitted.

Therefore, this change completely removes the separate serialization
mode. Instead, the remark string table is now always written to the end
of the remarks file. This requires us to tell the serializer when to
finalize remark serialization. This automatically happens when the
serializer goes out of scope. However, often the remark file goes out of
scope before the serializer is destroyed. To diagnose this, I have added
an assert to alert users that they need to explicitly call
finalizeLLVMOptimizationRemarks.

This change paves the way for further improvements to the remark
infrastructure, including more tooling (e.g. #159784), size optimizations
for bitstream remarks, and more.

Pull Request: #156715

Author: tobias-stadler

clang/lib/CodeGen/BackendUtil.cpp

@@ -29,6 +29,7 @@
 #include "llvm/Frontend/Driver/CodeGenOptions.h"
 #include "llvm/IR/DataLayout.h"
 #include "llvm/IR/DebugInfo.h"
+#include "llvm/IR/LLVMRemarkStreamer.h"
 #include "llvm/IR/LegacyPassManager.h"
 #include "llvm/IR/Module.h"
 #include "llvm/IR/ModuleSummaryIndex.h"
@@ -1384,6 +1385,10 @@ runThinLTOBackend(CompilerInstance &CI, ModuleSummaryIndex *CombinedIndex,
     Conf.CGFileType = getCodeGenFileType(Action);
     break;
   }
+
+  // FIXME: Both ExecuteAction and thinBackend set up optimization remarks for
+  // the same context.
+  finalizeLLVMOptimizationRemarks(M->getContext());
   if (Error E =
           thinBackend(Conf, -1, AddStream, *M, *CombinedIndex, ImportList,
                       ModuleToDefinedGVSummaries[M->getModuleIdentifier()],

clang/lib/CodeGen/CodeGenAction.cpp

@@ -29,6 +29,7 @@
 #include "clang/Lex/Preprocessor.h"
 #include "clang/Serialization/ASTWriter.h"
 #include "llvm/ADT/Hashing.h"
+#include "llvm/ADT/ScopeExit.h"
 #include "llvm/Bitcode/BitcodeReader.h"
 #include "llvm/CodeGen/MachineOptimizationRemarkEmitter.h"
 #include "llvm/Demangle/Demangle.h"
@@ -259,19 +260,18 @@ void BackendConsumer::HandleTranslationUnit(ASTContext &C) {
   Ctx.setDefaultTargetCPU(TargetOpts.CPU);
   Ctx.setDefaultTargetFeatures(llvm::join(TargetOpts.Features, ","));
 
-  Expected<std::unique_ptr<llvm::ToolOutputFile>> OptRecordFileOrErr =
-    setupLLVMOptimizationRemarks(
-      Ctx, CodeGenOpts.OptRecordFile, CodeGenOpts.OptRecordPasses,
-      CodeGenOpts.OptRecordFormat, CodeGenOpts.DiagnosticsWithHotness,
-      CodeGenOpts.DiagnosticsHotnessThreshold);
+  Expected<LLVMRemarkFileHandle> OptRecordFileOrErr =
+      setupLLVMOptimizationRemarks(
+          Ctx, CodeGenOpts.OptRecordFile, CodeGenOpts.OptRecordPasses,
+          CodeGenOpts.OptRecordFormat, CodeGenOpts.DiagnosticsWithHotness,
+          CodeGenOpts.DiagnosticsHotnessThreshold);
 
   if (Error E = OptRecordFileOrErr.takeError()) {
     reportOptRecordError(std::move(E), Diags, CodeGenOpts);
     return;
   }
 
-  std::unique_ptr<llvm::ToolOutputFile> OptRecordFile =
-    std::move(*OptRecordFileOrErr);
+  LLVMRemarkFileHandle OptRecordFile = std::move(*OptRecordFileOrErr);
 
   if (OptRecordFile && CodeGenOpts.getProfileUse() !=
                            llvm::driver::ProfileInstrKind::ProfileNone)
@@ -1173,7 +1173,7 @@ void CodeGenAction::ExecuteAction() {
   Ctx.setDefaultTargetCPU(TargetOpts.CPU);
   Ctx.setDefaultTargetFeatures(llvm::join(TargetOpts.Features, ","));
 
-  Expected<std::unique_ptr<llvm::ToolOutputFile>> OptRecordFileOrErr =
+  Expected<LLVMRemarkFileHandle> OptRecordFileOrErr =
       setupLLVMOptimizationRemarks(
           Ctx, CodeGenOpts.OptRecordFile, CodeGenOpts.OptRecordPasses,
           CodeGenOpts.OptRecordFormat, CodeGenOpts.DiagnosticsWithHotness,
@@ -1183,8 +1183,7 @@ void CodeGenAction::ExecuteAction() {
     reportOptRecordError(std::move(E), Diagnostics, CodeGenOpts);
     return;
   }
-  std::unique_ptr<llvm::ToolOutputFile> OptRecordFile =
-      std::move(*OptRecordFileOrErr);
+  LLVMRemarkFileHandle OptRecordFile = std::move(*OptRecordFileOrErr);
 
   emitBackendOutput(CI, CI.getCodeGenOpts(),
                     CI.getTarget().getDataLayoutString(), TheModule.get(), BA,

flang/lib/Frontend/FrontendActions.cpp

@@ -1352,7 +1352,7 @@ void CodeGenAction::executeAction() {
       std::make_unique<BackendRemarkConsumer>(remarkConsumer));
 
   // write optimization-record
-  llvm::Expected<std::unique_ptr<llvm::ToolOutputFile>> optRecordFileOrErr =
+  llvm::Expected<llvm::LLVMRemarkFileHandle> optRecordFileOrErr =
       setupLLVMOptimizationRemarks(
           llvmModule->getContext(), codeGenOpts.OptRecordFile,
           codeGenOpts.OptRecordPasses, codeGenOpts.OptRecordFormat,
@@ -1364,8 +1364,7 @@ void CodeGenAction::executeAction() {
     return;
   }
 
-  std::unique_ptr<llvm::ToolOutputFile> optRecordFile =
-      std::move(*optRecordFileOrErr);
+  llvm::LLVMRemarkFileHandle optRecordFile = std::move(*optRecordFileOrErr);
 
   if (optRecordFile) {
     optRecordFile->keep();

llvm/docs/Remarks.rst

@@ -152,26 +152,6 @@ Other tools that support remarks:
     .. option:: -opt-remarks-format=<format>
     .. option:: -opt-remarks-with-hotness
 
-Serialization modes
-===================
-
-There are two modes available for serializing remarks:
-
-``Separate``
-
-    In this mode, the remarks and the metadata are serialized separately. The
-    client is responsible for parsing the metadata first, then use the metadata
-    to correctly parse the remarks.
-
-``Standalone``
-
-    In this mode, the remarks and the metadata are serialized to the same
-    stream. The metadata will always come before the remarks.
-
-    The compiler does not support emitting standalone remarks. This mode is
-    more suited for post-processing tools like linkers, that can merge the
-    remarks for one whole project.
-
 .. _yamlremarks:
 
 YAML remarks
@@ -374,27 +354,11 @@ This block can contain the following records:
 The remark container
 --------------------
 
-Bitstream remarks are designed to be used in two different modes:
-
-``The separate mode``
-
-    The separate mode is the mode that is typically used during compilation. It
-    provides a way to serialize the remark entries to a stream while some
-    metadata is kept in memory to be emitted in the product of the compilation
-    (typically, an object file).
-
-``The standalone mode``
-
-    The standalone mode is typically stored and used after the distribution of
-    a program. It contains all the information that allows the parsing of all
-    the remarks without having any external dependencies.
-
-In order to support multiple modes, the format introduces the concept of a
-bitstream remark container type.
+The bitstream remark container supports multiple types:
 
-.. _bitstreamremarksseparateremarksmeta:
+.. _bitstreamremarksfileexternal:
 
-``SeparateRemarksMeta: the metadata emitted separately``
+``RemarksFileExternal: a link to an external remarks file``
 
     This container type expects only a :ref:`META_BLOCK <bitstreamremarksmetablock>` containing only:
 
@@ -406,84 +370,33 @@ bitstream remark container type.
     clients to retrieve remarks and their associated metadata directly from
     intermediate products.
 
-``SeparateRemarksFile: the remark entries emitted separately``
+    The container versions of the external separate container should match in order to
+    have a well-formed file.
 
-    This container type expects only a :ref:`META_BLOCK <bitstreamremarksmetablock>` containing only:
-
-    * :ref:`RECORD_META_CONTAINER_INFO <bitstreamremarksrecordmetacontainerinfo>`
-    * :ref:`RECORD_META_REMARK_VERSION <bitstreamremarksrecordmetaremarkversion>`
+.. _bitstreamremarksfile:
 
-    This container type expects 0 or more :ref:`REMARK_BLOCK <bitstreamremarksremarkblock>`.
+``RemarksFile: a standalone remarks file``
 
-    Typically, this is emitted in a side-file alongside an object file, and is
-    made to be able to stream to without increasing the memory consumption of
-    the compiler. This is referenced by the :ref:`RECORD_META_EXTERNAL_FILE
-    <bitstreamremarksrecordmetaexternalfile>` entry in the
-    :ref:`SeparateRemarksMeta <bitstreamremarksseparateremarksmeta>` container.
+    This container type expects a :ref:`META_BLOCK <bitstreamremarksmetablock>` containing only:
 
-When the parser tries to parse a container that contains the metadata for the
-separate remarks, it should parse the version and type, then keep the string
-table in memory while opening the external file, validating its metadata and
-parsing the remark entries.
+    * :ref:`RECORD_META_CONTAINER_INFO <bitstreamremarksrecordmetacontainerinfo>`
+    * :ref:`RECORD_META_REMARK_VERSION <bitstreamremarksrecordmetaremarkversion>`
 
-The container versions from the separate container should match in order to
-have a well-formed file.
+    Then, this container type expects 1 or more :ref:`REMARK_BLOCK <bitstreamremarksremarkblock>`.
+    If no remarks are emitted, the meta blocks are also not emitted, so the file is empty.
 
-``Standalone: the metadata and the remark entries emitted together``
+    After the remark blocks, another :ref:`META_BLOCK <bitstreamremarksmetablock>` is emitted, containing:
+    * :ref:`RECORD_META_STRTAB <bitstreamremarksrecordmetastrtab>`
 
-    This container type expects only a :ref:`META_BLOCK <bitstreamremarksmetablock>` containing only:
+    When the parser reads this container type, it jumps to the end of the file
+    to read the string table before parsing the individual remarks.
 
-    * :ref:`RECORD_META_CONTAINER_INFO <bitstreamremarksrecordmetacontainerinfo>`
-    * :ref:`RECORD_META_REMARK_VERSION <bitstreamremarksrecordmetaremarkversion>`
-    * :ref:`RECORD_META_STRTAB <bitstreamremarksrecordmetastrtab>`
+    Standalone remarks files can be referenced by the
+    :ref:`RECORD_META_EXTERNAL_FILE <bitstreamremarksrecordmetaexternalfile>`
+    entry in the :ref:`RemarksFileExternal
+    <bitstreamremarksfileexternal>` container.
 
-    This container type expects 0 or more :ref:`REMARK_BLOCK <bitstreamremarksremarkblock>`.
-
-A complete output of :program:`llvm-bcanalyzer` on the different container types:
-
-``SeparateRemarksMeta``
-
-.. code-block:: none
-
-    <BLOCKINFO_BLOCK/>
-    <Meta BlockID=8 NumWords=13 BlockCodeSize=3>
-      <Container info codeid=1 abbrevid=4 op0=5 op1=0/>
-      <String table codeid=3 abbrevid=5/> blob data = 'pass\\x00key\\x00value\\x00'
-      <External File codeid=4 abbrevid=6/> blob data = '/path/to/file/name'
-    </Meta>
-
-``SeparateRemarksFile``
-
-.. code-block:: none
-
-    <BLOCKINFO_BLOCK/>
-    <Meta BlockID=8 NumWords=3 BlockCodeSize=3>
-      <Container info codeid=1 abbrevid=4 op0=0 op1=1/>
-      <Remark version codeid=2 abbrevid=5 op0=0/>
-    </Meta>
-    <Remark BlockID=9 NumWords=8 BlockCodeSize=4>
-      <Remark header codeid=5 abbrevid=4 op0=2 op1=0 op2=1 op3=2/>
-      <Remark debug location codeid=6 abbrevid=5 op0=3 op1=99 op2=55/>
-      <Remark hotness codeid=7 abbrevid=6 op0=999999999/>
-      <Argument with debug location codeid=8 abbrevid=7 op0=4 op1=5 op2=6 op3=11 op4=66/>
-    </Remark>
-
-``Standalone``
-
-.. code-block:: none
-
-    <BLOCKINFO_BLOCK/>
-    <Meta BlockID=8 NumWords=15 BlockCodeSize=3>
-      <Container info codeid=1 abbrevid=4 op0=5 op1=2/>
-      <Remark version codeid=2 abbrevid=5 op0=30/>
-      <String table codeid=3 abbrevid=6/> blob data = 'pass\\x00remark\\x00function\\x00path\\x00key\\x00value\\x00argpath\\x00'
-    </Meta>
-    <Remark BlockID=9 NumWords=8 BlockCodeSize=4>
-      <Remark header codeid=5 abbrevid=4 op0=2 op1=1 op2=0 op3=2/>
-      <Remark debug location codeid=6 abbrevid=5 op0=3 op1=99 op2=55/>
-      <Remark hotness codeid=7 abbrevid=6 op0=999999999/>
-      <Argument with debug location codeid=8 abbrevid=7 op0=4 op1=5 op2=6 op3=11 op4=66/>
-    </Remark>
+.. FIXME: Add complete output of :program:`llvm-bcanalyzer` on the different container types (once format changes are completed)
 
 opt-viewer
 ==========

llvm/include/llvm/IR/LLVMRemarkStreamer.h

@@ -17,6 +17,7 @@
 #include "llvm/Remarks/Remark.h"
 #include "llvm/Support/Compiler.h"
 #include "llvm/Support/Error.h"
+#include "llvm/Support/ToolOutputFile.h"
 #include <memory>
 #include <optional>
 #include <string>
@@ -82,20 +83,81 @@ struct LLVMRemarkSetupFormatError
       LLVMRemarkSetupFormatError>::LLVMRemarkSetupErrorInfo;
 };
 
-/// Setup optimization remarks that output to a file.
-LLVM_ABI Expected<std::unique_ptr<ToolOutputFile>> setupLLVMOptimizationRemarks(
+/// RAII handle that manages the lifetime of the ToolOutputFile used to output
+/// remarks. On destruction (or when calling releaseFile()), this handle ensures
+/// that the optimization remarks are finalized and the RemarkStreamer is
+/// correctly deregistered from the LLVMContext.
+class LLVMRemarkFileHandle final {
+  struct Finalizer {
+    LLVMContext *Context;
+
+    Finalizer(LLVMContext *Ctx) : Context(Ctx) {}
+
+    Finalizer(const Finalizer &) = delete;
+    Finalizer &operator=(const Finalizer &) = delete;
+
+    Finalizer(Finalizer &&Other) : Context(Other.Context) {
+      Other.Context = nullptr;
+    }
+
+    Finalizer &operator=(Finalizer &&Other) {
+      std::swap(Context, Other.Context);
+      return *this;
+    }
+
+    ~Finalizer() { finalize(); }
+
+    LLVM_ABI void finalize();
+  };
+
+  std::unique_ptr<ToolOutputFile> OutputFile;
+  Finalizer Finalize;
+
+public:
+  LLVMRemarkFileHandle() : OutputFile(nullptr), Finalize(nullptr) {}
+
+  LLVMRemarkFileHandle(std::unique_ptr<ToolOutputFile> OutputFile,
+                       LLVMContext &Ctx)
+      : OutputFile(std::move(OutputFile)), Finalize(&Ctx) {}
+
+  ToolOutputFile *get() { return OutputFile.get(); }
+  explicit operator bool() { return bool(OutputFile); }
+
+  /// Finalize remark emission and release the underlying ToolOutputFile.
+  std::unique_ptr<ToolOutputFile> releaseFile() {
+    finalize();
+    return std::move(OutputFile);
+  }
+
+  void finalize() { Finalize.finalize(); }
+
+  ToolOutputFile &operator*() { return *OutputFile; }
+  ToolOutputFile *operator->() { return &*OutputFile; }
+};
+
+/// Set up optimization remarks that output to a file. The LLVMRemarkFileHandle
+/// manages the lifetime of the underlying ToolOutputFile to ensure \ref
+/// finalizeLLVMOptimizationRemarks() is called before the file is destroyed or
+/// released from the handle. The handle must be kept alive until all remarks
+/// were emitted through the remark streamer.
+LLVM_ABI Expected<LLVMRemarkFileHandle> setupLLVMOptimizationRemarks(
     LLVMContext &Context, StringRef RemarksFilename, StringRef RemarksPasses,
     StringRef RemarksFormat, bool RemarksWithHotness,
     std::optional<uint64_t> RemarksHotnessThreshold = 0);
 
-/// Setup optimization remarks that output directly to a raw_ostream.
-/// \p OS is managed by the caller and should be open for writing as long as \p
-/// Context is streaming remarks to it.
+/// Set up optimization remarks that output directly to a raw_ostream.
+/// \p OS is managed by the caller and must be open for writing until
+/// \ref finalizeLLVMOptimizationRemarks() is called.
 LLVM_ABI Error setupLLVMOptimizationRemarks(
     LLVMContext &Context, raw_ostream &OS, StringRef RemarksPasses,
     StringRef RemarksFormat, bool RemarksWithHotness,
     std::optional<uint64_t> RemarksHotnessThreshold = 0);
 
+/// Finalize optimization remarks and deregister the RemarkStreamer from the \p
+/// Context. This must be called before closing the (file) stream that was used
+/// to set up the remarks.
+LLVM_ABI void finalizeLLVMOptimizationRemarks(LLVMContext &Context);
+
 } // end namespace llvm
 
 #endif // LLVM_IR_LLVMREMARKSTREAMER_H

llvm/include/llvm/LTO/LTO.h

@@ -15,6 +15,7 @@
 #ifndef LLVM_LTO_LTO_H
 #define LLVM_LTO_LTO_H
 
+#include "llvm/IR/LLVMRemarkStreamer.h"
 #include "llvm/Support/Compiler.h"
 #include <memory>
 
@@ -91,7 +92,7 @@ LLVM_ABI std::string getThinLTOOutputFile(StringRef Path, StringRef OldPrefix,
                                           StringRef NewPrefix);
 
 /// Setup optimization remarks.
-LLVM_ABI Expected<std::unique_ptr<ToolOutputFile>> setupLLVMOptimizationRemarks(
+LLVM_ABI Expected<LLVMRemarkFileHandle> setupLLVMOptimizationRemarks(
     LLVMContext &Context, StringRef RemarksFilename, StringRef RemarksPasses,
     StringRef RemarksFormat, bool RemarksWithHotness,
     std::optional<uint64_t> RemarksHotnessThreshold = 0, int Count = -1);
@@ -579,7 +580,7 @@ class LTO {
   DenseSet<GlobalValue::GUID> DynamicExportSymbols;
 
   // Diagnostic optimization remarks file
-  std::unique_ptr<ToolOutputFile> DiagnosticOutputFile;
+  LLVMRemarkFileHandle DiagnosticOutputFile;
 };
 
 /// The resolution for a symbol. The linker must provide a SymbolResolution for

llvm/include/llvm/LTO/LTOBackend.h

@@ -65,8 +65,7 @@ thinBackend(const Config &C, unsigned Task, AddStreamFn AddStream, Module &M,
             AddStreamFn IRAddStream = nullptr,
             const std::vector<uint8_t> &CmdArgs = std::vector<uint8_t>());
 
-LLVM_ABI Error
-finalizeOptimizationRemarks(std::unique_ptr<ToolOutputFile> DiagOutputFile);
+LLVM_ABI Error finalizeOptimizationRemarks(LLVMRemarkFileHandle DiagOutputFile);
 
 /// Returns the BitcodeModule that is ThinLTO.
 LLVM_ABI BitcodeModule *findThinLTOModule(MutableArrayRef<BitcodeModule> BMs);

llvm/include/llvm/LTO/legacy/LTOCodeGenerator.h

@@ -244,7 +244,7 @@ struct LTOCodeGenerator {
   bool ShouldInternalize = EnableLTOInternalization;
   bool ShouldEmbedUselists = false;
   bool ShouldRestoreGlobalsLinkage = false;
-  std::unique_ptr<ToolOutputFile> DiagnosticOutputFile;
+  LLVMRemarkFileHandle DiagnosticOutputFile;
   std::unique_ptr<ToolOutputFile> StatsFile = nullptr;
   std::string SaveIRBeforeOptPath;