[Remarks] Introduce a new format: yaml-strtab

This exposes better support to use a string table with a format through
an actual new remark::Format, called yaml-strtab.

This can now be used with -fsave-optimization-record=yaml-strtab.

llvm-svn: 366849

docs/Remarks.rst

@@ -112,6 +112,7 @@ following options:
       Supported formats:
 
       * :ref:`yaml <yamlremarks>` (default)
+      * :ref:`yaml-strtab <yamlstrtabremarks>`
 
 ``Content configuration``
 
@@ -191,12 +192,40 @@ fields are required:
 * ``<arg-line>``
 * ``<arg-column>``
 
+.. _yamlstrtabremarks:
+
+YAML with a string table
+------------------------
+
+The YAML serialization supports the usage of a string table by using the
+``yaml-strtab`` format.
+
+This format replaces strings in the YAML output with integers representing the
+index in the string table that can be provided separately through metadata.
+
+The following entries can take advantage of the string table while respecting
+YAML rules:
+
+* ``<pass>``
+* ``<name>``
+* ``<function>``
+* ``<file>``
+* ``<value>``
+* ``<arg-file>``
+
+Currently, none of the tools in :ref:`the opt-viewer directory <optviewer>`
+support this format.
+
+.. _optviewer:
+
 opt-viewer
 ==========
 
 The ``opt-viewer`` directory contains a collection of tools that visualize and
 summarize serialized remarks.
 
+The tools only support the ``yaml`` format.
+
 .. _optviewerpy:
 
 opt-viewer.py

include/llvm/Remarks/RemarkFormat.h

@@ -22,7 +22,7 @@ namespace remarks {
 constexpr StringRef Magic("REMARKS", 7);
 
 /// The format used for serializing/deserializing remarks.
-enum class Format { Unknown, YAML };
+enum class Format { Unknown, YAML, YAMLStrTab };
 
 /// Parse and validate a string for the remark format.
 Expected<Format> parseFormat(StringRef FormatStr);

include/llvm/Remarks/RemarkParser.h

@@ -67,9 +67,12 @@ struct ParsedStringTable {
   ParsedStringTable(StringRef Buffer);
 };
 
+Expected<std::unique_ptr<Parser>> createRemarkParser(Format ParserFormat,
+                                                     StringRef Buf);
+
 Expected<std::unique_ptr<Parser>>
 createRemarkParser(Format ParserFormat, StringRef Buf,
-                   Optional<const ParsedStringTable *> StrTab = None);
+                   const ParsedStringTable &StrTab);
 
 } // end namespace remarks
 } // end namespace llvm

include/llvm/Remarks/RemarkSerializer.h

@@ -36,9 +36,6 @@ struct Serializer {
   virtual void emit(const Remark &Remark) = 0;
 };
 
-/// Wether the serializer should use a string table while emitting.
-enum class UseStringTable { No, Yes };
-
 } // end namespace remarks
 } // end namespace llvm
 

include/llvm/Remarks/YAMLRemarkSerializer.h

@@ -34,13 +34,22 @@ struct YAMLSerializer : public Serializer {
   /// The YAML streamer.
   yaml::Output YAMLOutput;
 
-  YAMLSerializer(raw_ostream &OS,
-                 UseStringTable UseStringTable = remarks::UseStringTable::No);
+  YAMLSerializer(raw_ostream &OS);
 
   /// Emit a remark to the stream.
   void emit(const Remark &Remark) override;
 };
 
+/// Serialize the remarks to YAML using a string table. An remark entry looks
+/// like the regular YAML remark but instead of string entries it's using
+/// numbers that map to an index in the string table.
+struct YAMLStrTabSerializer : public YAMLSerializer {
+  YAMLStrTabSerializer(raw_ostream &OS) : YAMLSerializer(OS) {
+    // Having a string table set up enables the serializer to use it.
+    StrTab.emplace();
+  }
+};
+
 } // end namespace remarks
 } // end namespace llvm
 

lib/IR/RemarkStreamer.cpp

@@ -110,11 +110,13 @@ char RemarkSetupFormatError::ID = 0;
 static std::unique_ptr<remarks::Serializer>
 formatToSerializer(remarks::Format RemarksFormat, raw_ostream &OS) {
   switch (RemarksFormat) {
-  default:
+  case remarks::Format::Unknown:
     llvm_unreachable("Unknown remark serializer format.");
     return nullptr;
   case remarks::Format::YAML:
     return llvm::make_unique<remarks::YAMLSerializer>(OS);
+  case remarks::Format::YAMLStrTab:
+    return llvm::make_unique<remarks::YAMLStrTabSerializer>(OS);
   };
 }
 

lib/Remarks/RemarkFormat.cpp

@@ -19,6 +19,7 @@ using namespace llvm::remarks;
 Expected<Format> llvm::remarks::parseFormat(StringRef FormatStr) {
   auto Result = StringSwitch<Format>(FormatStr)
                     .Cases("", "yaml", Format::YAML)
+                    .Cases("", "yaml-strtab", Format::YAMLStrTab)
                     .Default(Format::Unknown);
 
   if (Result == Format::Unknown)

lib/Remarks/RemarkParser.cpp

@@ -48,16 +48,34 @@ Expected<StringRef> ParsedStringTable::operator[](size_t Index) const {
 }
 
 Expected<std::unique_ptr<Parser>>
-llvm::remarks::createRemarkParser(Format ParserFormat, StringRef Buf,
-                                  Optional<const ParsedStringTable *> StrTab) {
+llvm::remarks::createRemarkParser(Format ParserFormat, StringRef Buf) {
   switch (ParserFormat) {
   case Format::YAML:
-    return llvm::make_unique<YAMLRemarkParser>(Buf, StrTab);
+    return llvm::make_unique<YAMLRemarkParser>(Buf);
+  case Format::YAMLStrTab:
+    return createStringError(
+        std::make_error_code(std::errc::invalid_argument),
+        "The YAML with string table format requires a parsed string table.");
+  case Format::Unknown:
+    return createStringError(std::make_error_code(std::errc::invalid_argument),
+                             "Unknown remark parser format.");
+  }
+}
+
+Expected<std::unique_ptr<Parser>>
+llvm::remarks::createRemarkParser(Format ParserFormat, StringRef Buf,
+                                  const ParsedStringTable &StrTab) {
+  switch (ParserFormat) {
+  case Format::YAML:
+    return createStringError(std::make_error_code(std::errc::invalid_argument),
+                             "The YAML format can't be used with a string "
+                             "table. Use yaml-strtab instead.");
+  case Format::YAMLStrTab:
+    return llvm::make_unique<YAMLStrTabRemarkParser>(Buf, StrTab);
   case Format::Unknown:
     return createStringError(std::make_error_code(std::errc::invalid_argument),
                              "Unknown remark parser format.");
   }
-  llvm_unreachable("unknown format");
 }
 
 // Wrapper that holds the state needed to interact with the C API.
@@ -67,7 +85,9 @@ struct CParser {
 
   CParser(Format ParserFormat, StringRef Buf,
           Optional<const ParsedStringTable *> StrTab = None)
-      : TheParser(cantFail(createRemarkParser(ParserFormat, Buf, StrTab))) {}
+      : TheParser(cantFail(StrTab
+                               ? createRemarkParser(ParserFormat, Buf, **StrTab)
+                               : createRemarkParser(ParserFormat, Buf))) {}
 
   void handleError(Error E) { Err.emplace(toString(std::move(E))); }
   bool hasError() const { return Err.hasValue(); }

lib/Remarks/YAMLRemarkParser.cpp

@@ -54,6 +54,9 @@ static SourceMgr setupSM(std::string &LastErrorMessage) {
   return SM;
 }
 
+YAMLRemarkParser::YAMLRemarkParser(StringRef Buf)
+    : YAMLRemarkParser(Buf, None) {}
+
 YAMLRemarkParser::YAMLRemarkParser(StringRef Buf,
                                    Optional<const ParsedStringTable *> StrTab)
     : Parser{Format::YAML}, StrTab(StrTab), LastErrorMessage(),
@@ -179,22 +182,7 @@ Expected<StringRef> YAMLRemarkParser::parseStr(yaml::KeyValueNode &Node) {
   auto *Value = dyn_cast<yaml::ScalarNode>(Node.getValue());
   if (!Value)
     return error("expected a value of scalar type.", Node);
-  StringRef Result;
-  if (!StrTab) {
-    Result = Value->getRawValue();
-  } else {
-    // If we have a string table, parse it as an unsigned.
-    unsigned StrID = 0;
-    if (Expected<unsigned> MaybeStrID = parseUnsigned(Node))
-      StrID = *MaybeStrID;
-    else
-      return MaybeStrID.takeError();
-
-    if (Expected<StringRef> Str = (**StrTab)[StrID])
-      Result = *Str;
-    else
-      return Str.takeError();
-  }
+  StringRef Result = Value->getRawValue();
 
   if (Result.front() == '\'')
     Result = Result.drop_front();
@@ -325,3 +313,29 @@ Expected<std::unique_ptr<Remark>> YAMLRemarkParser::next() {
 
   return std::move(*MaybeResult);
 }
+
+Expected<StringRef> YAMLStrTabRemarkParser::parseStr(yaml::KeyValueNode &Node) {
+  auto *Value = dyn_cast<yaml::ScalarNode>(Node.getValue());
+  if (!Value)
+    return error("expected a value of scalar type.", Node);
+  StringRef Result;
+  // If we have a string table, parse it as an unsigned.
+  unsigned StrID = 0;
+  if (Expected<unsigned> MaybeStrID = parseUnsigned(Node))
+    StrID = *MaybeStrID;
+  else
+    return MaybeStrID.takeError();
+
+  if (Expected<StringRef> Str = (**StrTab)[StrID])
+    Result = *Str;
+  else
+    return Str.takeError();
+
+  if (Result.front() == '\'')
+    Result = Result.drop_front();
+
+  if (Result.back() == '\'')
+    Result = Result.drop_back();
+
+  return Result;
+}

lib/Remarks/YAMLRemarkParser.h

@@ -59,8 +59,7 @@ struct YAMLRemarkParser : public Parser {
   /// Iterator in the YAML stream.
   yaml::document_iterator YAMLIt;
 
-  YAMLRemarkParser(StringRef Buf,
-                   Optional<const ParsedStringTable *> StrTab = None);
+  YAMLRemarkParser(StringRef Buf);
 
   Expected<std::unique_ptr<Remark>> next() override;
 
@@ -68,7 +67,8 @@ struct YAMLRemarkParser : public Parser {
     return P->ParserFormat == Format::YAML;
   }
 
-private:
+protected:
+  YAMLRemarkParser(StringRef Buf, Optional<const ParsedStringTable *> StrTab);
   /// Create a YAMLParseError error from an existing error generated by the YAML
   /// parser.
   /// If there is no error, this returns Success.
@@ -82,7 +82,7 @@ private:
   /// Parse one key to a string.
   Expected<StringRef> parseKey(yaml::KeyValueNode &Node);
   /// Parse one value to a string.
-  Expected<StringRef> parseStr(yaml::KeyValueNode &Node);
+  virtual Expected<StringRef> parseStr(yaml::KeyValueNode &Node);
   /// Parse one value to an unsigned.
   Expected<unsigned> parseUnsigned(yaml::KeyValueNode &Node);
   /// Parse a debug location.
@@ -90,6 +90,20 @@ private:
   /// Parse an argument.
   Expected<Argument> parseArg(yaml::Node &Node);
 };
+
+/// YAML with a string table to Remark parser.
+struct YAMLStrTabRemarkParser : public YAMLRemarkParser {
+  YAMLStrTabRemarkParser(StringRef Buf, const ParsedStringTable &StrTab)
+      : YAMLRemarkParser(Buf, &StrTab) {}
+
+  static bool classof(const Parser *P) {
+    return P->ParserFormat == Format::YAMLStrTab;
+  }
+
+protected:
+  /// Parse one value to a string.
+  Expected<StringRef> parseStr(yaml::KeyValueNode &Node) override;
+};
 } // end namespace remarks
 } // end namespace llvm
 

lib/Remarks/YAMLRemarkSerializer.cpp

@@ -17,10 +17,6 @@
 using namespace llvm;
 using namespace llvm::remarks;
 
-cl::opt<bool> RemarksYAMLStringTable(
-    "remarks-yaml-string-table", cl::init(false), cl::Hidden,
-    cl::desc("Enable the usage of a string table with YAML remarks."));
-
 // Use the same keys whether we use a string table or not (respectively, T is an
 // unsigned or a StringRef).
 template <typename T>
@@ -153,11 +149,8 @@ template <> struct MappingTraits<Argument> {
 
 LLVM_YAML_IS_SEQUENCE_VECTOR(Argument)
 
-YAMLSerializer::YAMLSerializer(raw_ostream &OS, UseStringTable UseStringTable)
-    : Serializer(OS), YAMLOutput(OS, reinterpret_cast<void *>(this)) {
-  if (UseStringTable == remarks::UseStringTable::Yes || RemarksYAMLStringTable)
-    StrTab.emplace();
-}
+YAMLSerializer::YAMLSerializer(raw_ostream &OS)
+    : Serializer(OS), YAMLOutput(OS, reinterpret_cast<void *>(this)) {}
 
 void YAMLSerializer::emit(const Remark &Remark) {
   // Again, YAMLTraits expect a non-const object for inputting, but we're not

test/CodeGen/X86/remarks-section.ll

@@ -1,6 +1,6 @@
 ; RUN: llc < %s -mtriple=x86_64-linux -remarks-section -pass-remarks-output=%/t.yaml | FileCheck -DPATH=%/t.yaml %s
 ; RUN: llc < %s -mtriple=x86_64-darwin -remarks-section -pass-remarks-output=%/t.yaml | FileCheck --check-prefix=CHECK-DARWIN -DPATH=%/t.yaml %s
-; RUN: llc < %s -mtriple=x86_64-darwin -remarks-section -remarks-yaml-string-table -pass-remarks-output=%/t.yaml | FileCheck --check-prefix=CHECK-DARWIN-STRTAB -DPATH=%/t.yaml %s
+; RUN: llc < %s -mtriple=x86_64-darwin --pass-remarks-format=yaml-strtab -remarks-section -pass-remarks-output=%/t.yaml | FileCheck --check-prefix=CHECK-DARWIN-STRTAB -DPATH=%/t.yaml %s
 
 ; CHECK-LABEL: func1:
 

unittests/Remarks/YAMLRemarksParsingTest.cpp

@@ -526,7 +526,7 @@ TEST(YAMLRemarks, ContentsStrTab) {
 
   remarks::ParsedStringTable StrTab(StrTabBuf);
   Expected<std::unique_ptr<remarks::Parser>> MaybeParser =
-      remarks::createRemarkParser(remarks::Format::YAML, Buf, &StrTab);
+      remarks::createRemarkParser(remarks::Format::YAMLStrTab, Buf, StrTab);
   EXPECT_FALSE(errorToBool(MaybeParser.takeError()));
   EXPECT_TRUE(*MaybeParser != nullptr);
 
@@ -601,7 +601,7 @@ TEST(YAMLRemarks, ParsingBadStringTableIndex) {
 
   remarks::ParsedStringTable StrTab(StrTabBuf);
   Expected<std::unique_ptr<remarks::Parser>> MaybeParser =
-      remarks::createRemarkParser(remarks::Format::YAML, Buf, &StrTab);
+      remarks::createRemarkParser(remarks::Format::YAMLStrTab, Buf, StrTab);
   EXPECT_FALSE(errorToBool(MaybeParser.takeError()));
   EXPECT_TRUE(*MaybeParser != nullptr);
 

unittests/Remarks/YAMLRemarksSerializerTest.cpp

@@ -16,16 +16,17 @@ static void check(const remarks::Remark &R, StringRef Expected,
                   Optional<StringRef> ExpectedStrTab = None) {
   std::string Buf;
   raw_string_ostream OS(Buf);
-  remarks::UseStringTable UseStrTab = ExpectedStrTab.hasValue()
-                                          ? remarks::UseStringTable::Yes
-                                          : remarks::UseStringTable::No;
-  remarks::YAMLSerializer S(OS, UseStrTab);
-  S.emit(R);
+  bool UseStrTab = ExpectedStrTab.hasValue();
+  std::unique_ptr<remarks::Serializer> S =
+      UseStrTab ? llvm::make_unique<remarks::YAMLStrTabSerializer>(OS)
+                : llvm::make_unique<remarks::YAMLSerializer>(OS);
+
+  S->emit(R);
   EXPECT_EQ(OS.str(), Expected);
   if (ExpectedStrTab) {
     Buf.clear();
-    EXPECT_TRUE(S.StrTab);
-    S.StrTab->serialize(OS);
+    EXPECT_TRUE(S->StrTab);
+    S->StrTab->serialize(OS);
     EXPECT_EQ(OS.str(), *ExpectedStrTab);
   }
 }