[MLIR] Make generated markdown doc more consistent #119926

GleasonK · 2024-12-13T21:11:54Z

A few changes to doc generation:

All summaries are in italics.
In general each optional block starts and ends with a newline.
All table elements are enclosed in |'s
Overall reduce the number of >2newlines in a row

Rationale for this change is that our markdown to docs generator requires a newline before all headers, otherwise it gets inlined into the line before it, see ### sdy-op-priority-propagate in the image below.

That said overall I feel this formatting is more consistent now, here's a before and after:

Dialect documentation diff: https://www.diffchecker.com/OVMHoXeL/
Pass documentation diff: https://www.diffchecker.com/XEJRmW3k/

llvmbot · 2024-12-13T21:12:32Z

@llvm/pr-subscribers-tablegen
@llvm/pr-subscribers-mlir

@llvm/pr-subscribers-mlir-core

Author: Kevin Gleason (GleasonK)

Changes

A few changes to doc generation:

All summaries are in italics.
In general each optional block starts and ends with a newline.
All table elements are enclosed in |'s
Overall reduce the number of >2newlines in a row

Rationale for this change is that our markdown to docs generator requires a newline before all headers, otherwise it gets inlined into the line before it, see ### sdy-op-priority-propagate in the image below.

That said overall I feel this formatting is more consistent now, here's a before and after:

Dialect documentation diff: https://www.diffchecker.com/OVMHoXeL/
Pass documentation diff: https://www.diffchecker.com/XEJRmW3k/

Full diff: https://github.com/llvm/llvm-project/pull/119926.diff

5 Files Affected:

(modified) mlir/test/mlir-tblgen/gen-dialect-doc.td (+22-2)
(added) mlir/test/mlir-tblgen/gen-pass-doc.td (+20)
(modified) mlir/tools/mlir-tblgen/OpDocGen.cpp (+44-48)
(modified) mlir/tools/mlir-tblgen/OpInterfacesGen.cpp (+5-5)
(modified) mlir/tools/mlir-tblgen/PassDocGen.cpp (+3-3)

diff --git a/mlir/test/mlir-tblgen/gen-dialect-doc.td b/mlir/test/mlir-tblgen/gen-dialect-doc.td
index 79d755111e8f67..06c90b702278eb 100644
--- a/mlir/test/mlir-tblgen/gen-dialect-doc.td
+++ b/mlir/test/mlir-tblgen/gen-dialect-doc.td
@@ -36,7 +36,15 @@ def ACOp : Op<Test_Dialect, "c", [NoMemoryEffect, SingleBlockImplicitTerminator<
 def ABOp : Op<Test_Dialect, "b", [NoMemoryEffect, SingleBlockImplicitTerminator<"YieldOp">]>;
 }
 
-def AEOp : Op<Test_Dialect, "e", [NoMemoryEffect, SingleBlockImplicitTerminator<"YieldOp">]>;
+def AEOp : Op<Test_Dialect, "e", [NoMemoryEffect]> {
+  let summary = "Op with a summary";
+  let description = "Op with a description";
+  let arguments = (ins ConfinedType<AnyType, [CPred<"::llvm::isa<::mlir::TensorType>($_self)">]>:$tensor,
+                       I16Attr:$int_attr);
+  let results = (outs
+    ConfinedType<AnyType, [CPred<"::llvm::isa<::mlir::TensorType>($_self)">]>:$output
+  ); 
+}
 
 def TestAttr : DialectAttr<Test_Dialect, CPred<"true">> {
   let summary = "attribute summary";
@@ -85,7 +93,19 @@ def TestEnum :
 // CHECK: [TOC]
 
 // CHECK-NOT: [TOC]
+
 // CHECK: test.e
+// CHECK: _Op with a summary_
+// CHECK: Op with a description
+// CHECK: Operands:
+// CHECK: | Operand | Description |
+// CHECK: | :-----: | ----------- |
+// CHECK: | `tensor` | |
+// CHECK: Results:
+// CHECK: | Result | Description |
+// CHECK: | :----: | ----------- |
+// CHECK: | `output` | |
+
 // CHECK: Group of ops
 // CHECK: test.a
 // CHECK: test.d
@@ -122,7 +142,7 @@ def TestEnum :
 
 // CHECK: ## Enums
 // CHECK: ### TestEnum
-// CHECK: enum summary
+// CHECK: _Enum summary_
 // CHECK: #### Cases:
 // CHECK: | Symbol | Value | String |
 // CHECK: | :----: | :---: | ------ |
diff --git a/mlir/test/mlir-tblgen/gen-pass-doc.td b/mlir/test/mlir-tblgen/gen-pass-doc.td
new file mode 100644
index 00000000000000..6c3aeabc508457
--- /dev/null
+++ b/mlir/test/mlir-tblgen/gen-pass-doc.td
@@ -0,0 +1,20 @@
+// RUN: mlir-tblgen -gen-pass-doc -I %S/../../include -dialect=test %s | FileCheck %s
+
+include "mlir/Pass/PassBase.td"
+
+def TestPassDoc : Pass<"test-pass-doc"> {
+  let summary = "pass summary";
+  let description = [{
+    Pass description
+  }];
+
+  let options = [
+    ListOption<"option", "option", "std::string", "pass option">
+  ];
+}
+
+// CHECK: `-test-pass-doc`
+// CHECK: _Pass summary_
+// CHECK: Pass description
+// CHECK: Options
+// CHECK: -option : pass option
diff --git a/mlir/tools/mlir-tblgen/OpDocGen.cpp b/mlir/tools/mlir-tblgen/OpDocGen.cpp
index d499c78a5cf44d..a6e92209c9c57c 100644
--- a/mlir/tools/mlir-tblgen/OpDocGen.cpp
+++ b/mlir/tools/mlir-tblgen/OpDocGen.cpp
@@ -54,12 +54,12 @@ cl::opt<bool> allowHugoSpecificFeatures(
     cl::cat(docCat));
 
 void mlir::tblgen::emitSummary(StringRef summary, raw_ostream &os) {
-  if (!summary.empty()) {
-    StringRef trimmed = summary.trim();
-    char first = std::toupper(trimmed.front());
-    StringRef rest = trimmed.drop_front();
-    os << "\n_" << first << rest << "_\n\n";
-  }
+  if (summary.empty())
+    return;
+  StringRef trimmed = summary.trim();
+  char first = std::toupper(trimmed.front());
+  StringRef rest = trimmed.drop_front();
+  os << "\n_" << first << rest << "_\n";
 }
 
 // Emit the description by aligning the text to the left per line (e.g.,
@@ -69,6 +69,9 @@ void mlir::tblgen::emitSummary(StringRef summary, raw_ostream &os) {
 // in a way the user wanted but has some additional indenting due to being
 // nested in the op definition.
 void mlir::tblgen::emitDescription(StringRef description, raw_ostream &os) {
+  if (description.empty())
+    return;
+  os << "\n";
   raw_indented_ostream ros(os);
   StringRef trimmed = description.rtrim(" \t");
   ros.printReindented(trimmed);
@@ -80,6 +83,7 @@ void mlir::tblgen::emitDescriptionComment(StringRef description,
                                           raw_ostream &os, StringRef prefix) {
   if (description.empty())
     return;
+  os << "\n";
   raw_indented_ostream ros(os);
   StringRef trimmed = description.rtrim(" \t");
   ros.printReindented(trimmed, (Twine(prefix) + "/// ").str());
@@ -87,22 +91,14 @@ void mlir::tblgen::emitDescriptionComment(StringRef description,
     ros << "\n";
 }
 
-// Emits `str` with trailing newline if not empty.
-static void emitIfNotEmpty(StringRef str, raw_ostream &os) {
-  if (!str.empty()) {
-    emitDescription(str, os);
-    os << "\n";
-  }
-}
-
 /// Emit the given named constraint.
 template <typename T>
 static void emitNamedConstraint(const T &it, raw_ostream &os) {
   if (!it.name.empty())
     os << "| `" << it.name << "`";
   else
-    os << "&laquo;unnamed&raquo;";
-  os << " | " << it.constraint.getSummary() << "\n";
+    os << "| &laquo;unnamed&raquo;";
+  os << " | " << it.constraint.getSummary() << " |\n";
 }
 
 //===----------------------------------------------------------------------===//
@@ -112,6 +108,8 @@ static void emitNamedConstraint(const T &it, raw_ostream &os) {
 /// Emit the assembly format of an operation.
 static void emitAssemblyFormat(StringRef opName, StringRef format,
                                raw_ostream &os) {
+  if (format.empty())
+    return;
   os << "\nSyntax:\n\n```\noperation ::= `" << opName << "` ";
 
   // Print the assembly format aligned.
@@ -124,7 +122,7 @@ static void emitAssemblyFormat(StringRef opName, StringRef format,
     if (!formatChunk.empty())
       os.indent(indent) << formatChunk << "\n";
   } while (!split.second.empty());
-  os << "```\n\n";
+  os << "```\n";
 }
 
 /// Place `text` between backticks so that the Markdown processor renders it as
@@ -199,7 +197,7 @@ static void emitOpDoc(const Operator &op, raw_ostream &os) {
   std::string classNameStr = op.getQualCppClassName();
   StringRef className = classNameStr;
   (void)className.consume_front(stripPrefix);
-  os << formatv("### `{0}` ({1})\n", op.getOperationName(), className);
+  os << formatv("\n### `{0}` ({1})\n", op.getOperationName(), className);
 
   // Emit the summary, syntax, and description if present.
   if (op.hasSummary())
@@ -281,8 +279,8 @@ static void emitSourceLink(StringRef inputFilename, raw_ostream &os) {
 
   StringRef inputFromMlirInclude = inputFilename.substr(pathBegin);
 
-  os << "[source](https://github.com/llvm/llvm-project/blob/main/"
-     << inputFromMlirInclude << ")\n\n";
+  os << "\n[source](https://github.com/llvm/llvm-project/blob/main/"
+     << inputFromMlirInclude << ")\n";
 }
 
 static void emitOpDoc(const RecordKeeper &records, raw_ostream &os) {
@@ -299,9 +297,9 @@ static void emitOpDoc(const RecordKeeper &records, raw_ostream &os) {
 //===----------------------------------------------------------------------===//
 
 static void emitAttrDoc(const Attribute &attr, raw_ostream &os) {
-  os << "### " << attr.getSummary() << "\n\n";
+  os << "\n### " << attr.getSummary() << "\n";
   emitDescription(attr.getDescription(), os);
-  os << "\n\n";
+  os << "\n";
 }
 
 //===----------------------------------------------------------------------===//
@@ -309,9 +307,9 @@ static void emitAttrDoc(const Attribute &attr, raw_ostream &os) {
 //===----------------------------------------------------------------------===//
 
 static void emitTypeDoc(const Type &type, raw_ostream &os) {
-  os << "### " << type.getSummary() << "\n\n";
+  os << "\n### " << type.getSummary() << "\n";
   emitDescription(type.getDescription(), os);
-  os << "\n\n";
+  os << "\n";
 }
 
 //===----------------------------------------------------------------------===//
@@ -342,11 +340,11 @@ static void emitAttrOrTypeDefAssemblyFormat(const AttrOrTypeDef &def,
 }
 
 static void emitAttrOrTypeDefDoc(const AttrOrTypeDef &def, raw_ostream &os) {
-  os << formatv("### {0}\n", def.getCppClassName());
+  os << formatv("\n### {0}\n", def.getCppClassName());
 
   // Emit the summary if present.
   if (def.hasSummary())
-    os << "\n" << def.getSummary() << "\n";
+    emitSummary(def.getSummary(), os);
 
   // Emit the syntax if present.
   if (def.getMnemonic() && !def.hasCustomAssemblyFormat())
@@ -354,7 +352,6 @@ static void emitAttrOrTypeDefDoc(const AttrOrTypeDef &def, raw_ostream &os) {
 
   // Emit the description if present.
   if (def.hasDescription()) {
-    os << "\n";
     mlir::tblgen::emitDescription(def.getDescription(), os);
   }
 
@@ -363,11 +360,11 @@ static void emitAttrOrTypeDefDoc(const AttrOrTypeDef &def, raw_ostream &os) {
   if (!parameters.empty()) {
     os << "\n#### Parameters:\n\n";
     os << "| Parameter | C++ type | Description |\n"
-       << "| :-------: | :-------: | ----------- |\n";
+       << "| :-------: | :-------: | ----------- |";
     for (const auto &it : parameters) {
       auto desc = it.getSummary();
-      os << "| " << it.getName() << " | `" << it.getCppType() << "` | "
-         << (desc ? *desc : "") << " |\n";
+      os << "\n| " << it.getName() << " | `" << it.getCppType() << "` | "
+         << (desc ? *desc : "") << " |";
     }
   }
 
@@ -388,20 +385,19 @@ static void emitAttrOrTypeDefDoc(const RecordKeeper &records, raw_ostream &os,
 //===----------------------------------------------------------------------===//
 
 static void emitEnumDoc(const EnumAttr &def, raw_ostream &os) {
-  os << formatv("### {0}\n", def.getEnumClassName());
+  os << formatv("\n### {0}\n", def.getEnumClassName());
 
   // Emit the summary if present.
-  if (!def.getSummary().empty())
-    os << "\n" << def.getSummary() << "\n";
+  emitSummary(def.getSummary(), os);
 
   // Emit case documentation.
   std::vector<EnumAttrCase> cases = def.getAllCases();
   os << "\n#### Cases:\n\n";
   os << "| Symbol | Value | String |\n"
-     << "| :----: | :---: | ------ |\n";
+     << "| :----: | :---: | ------ |";
   for (const auto &it : cases) {
-    os << "| " << it.getSymbol() << " | `" << it.getValue() << "` | "
-       << it.getStr() << " |\n";
+    os << "\n| " << it.getSymbol() << " | `" << it.getValue() << "` | "
+       << it.getStr() << " |";
   }
 
   os << "\n";
@@ -447,7 +443,7 @@ static void emitBlock(ArrayRef<Attribute> attributes, StringRef inputFilename,
                       ArrayRef<Type> types, ArrayRef<TypeDef> typeDefs,
                       ArrayRef<EnumAttr> enums, raw_ostream &os) {
   if (!ops.empty()) {
-    os << "## Operations\n\n";
+    os << "\n## Operations\n";
     emitSourceLink(inputFilename, os);
     for (const OpDocGroup &grouping : ops) {
       bool nested = !grouping.summary.empty();
@@ -455,9 +451,9 @@ static void emitBlock(ArrayRef<Attribute> attributes, StringRef inputFilename,
           nested,
           [&](raw_ostream &os) {
             if (nested) {
-              os << "## " << StringRef(grouping.summary).trim() << "\n\n";
+              os << "\n## " << StringRef(grouping.summary).trim() << "\n";
               emitDescription(grouping.description, os);
-              os << "\n\n";
+              os << "\n";
             }
             for (const Operator &op : grouping.ops) {
               emitOpDoc(op, os);
@@ -468,32 +464,32 @@ static void emitBlock(ArrayRef<Attribute> attributes, StringRef inputFilename,
   }
 
   if (!attributes.empty()) {
-    os << "## Attribute constraints\n\n";
+    os << "\n## Attribute constraints\n";
     for (const Attribute &attr : attributes)
       emitAttrDoc(attr, os);
   }
 
   if (!attrDefs.empty()) {
-    os << "## Attributes\n\n";
+    os << "\n## Attributes\n";
     for (const AttrDef &def : attrDefs)
       emitAttrOrTypeDefDoc(def, os);
   }
 
   // TODO: Add link between use and def for types
   if (!types.empty()) {
-    os << "## Type constraints\n\n";
+    os << "\n## Type constraints\n";
     for (const Type &type : types)
       emitTypeDoc(type, os);
   }
 
   if (!typeDefs.empty()) {
-    os << "## Types\n\n";
+    os << "\n## Types\n";
     for (const TypeDef &def : typeDefs)
       emitAttrOrTypeDefDoc(def, os);
   }
 
   if (!enums.empty()) {
-    os << "## Enums\n\n";
+    os << "\n## Enums\n";
     for (const EnumAttr &def : enums)
       emitEnumDoc(def, os);
   }
@@ -504,14 +500,14 @@ static void emitDialectDoc(const Dialect &dialect, StringRef inputFilename,
                            ArrayRef<AttrDef> attrDefs, ArrayRef<OpDocGroup> ops,
                            ArrayRef<Type> types, ArrayRef<TypeDef> typeDefs,
                            ArrayRef<EnumAttr> enums, raw_ostream &os) {
-  os << "# '" << dialect.getName() << "' Dialect\n\n";
-  emitIfNotEmpty(dialect.getSummary(), os);
-  emitIfNotEmpty(dialect.getDescription(), os);
+  os << "\n# '" << dialect.getName() << "' Dialect\n";
+  emitSummary(dialect.getSummary(), os);
+  emitDescription(dialect.getDescription(), os);
 
   // Generate a TOC marker except if description already contains one.
   Regex r("^[[:space:]]*\\[TOC\\]$", Regex::RegexFlags::Newline);
   if (!r.match(dialect.getDescription()))
-    os << "[TOC]\n\n";
+    os << "\n[TOC]\n";
 
   emitBlock(attributes, inputFilename, attrDefs, ops, types, typeDefs, enums,
             os);
diff --git a/mlir/tools/mlir-tblgen/OpInterfacesGen.cpp b/mlir/tools/mlir-tblgen/OpInterfacesGen.cpp
index 1f1b1d9a340391..dcd68e6c2d6366 100644
--- a/mlir/tools/mlir-tblgen/OpInterfacesGen.cpp
+++ b/mlir/tools/mlir-tblgen/OpInterfacesGen.cpp
@@ -627,8 +627,8 @@ static void emitInterfaceDoc(const Record &interfaceDef, raw_ostream &os) {
   Interface interface(&interfaceDef);
 
   // Emit the interface name followed by the description.
-  os << "## " << interface.getName() << " (`" << interfaceDef.getName()
-     << "`)\n\n";
+  os << "\n## " << interface.getName() << " (`" << interfaceDef.getName()
+     << "`)\n";
   if (auto description = interface.getDescription())
     mlir::tblgen::emitDescription(*description, os);
 
@@ -636,7 +636,7 @@ static void emitInterfaceDoc(const Record &interfaceDef, raw_ostream &os) {
   os << "\n### Methods:\n";
   for (const auto &method : interface.getMethods()) {
     // Emit the method name.
-    os << "#### `" << method.getName() << "`\n\n```c++\n";
+    os << "\n#### `" << method.getName() << "`\n\n```c++\n";
 
     // Emit the method signature.
     if (method.isStatic())
@@ -656,13 +656,13 @@ static void emitInterfaceDoc(const Record &interfaceDef, raw_ostream &os) {
     if (!method.getBody())
       os << "\nNOTE: This method *must* be implemented by the user.";
 
-    os << "\n\n";
+    os << "\n";
   }
 }
 
 bool InterfaceGenerator::emitInterfaceDocs() {
   os << "<!-- Autogenerated by mlir-tblgen; don't manually edit -->\n";
-  os << "# " << interfaceBaseType << " definitions\n";
+  os << "\n# " << interfaceBaseType << " definitions\n";
 
   for (const auto *def : defs)
     emitInterfaceDoc(*def, os);
diff --git a/mlir/tools/mlir-tblgen/PassDocGen.cpp b/mlir/tools/mlir-tblgen/PassDocGen.cpp
index a2cb514ece3eba..456f9ceffeb9b2 100644
--- a/mlir/tools/mlir-tblgen/PassDocGen.cpp
+++ b/mlir/tools/mlir-tblgen/PassDocGen.cpp
@@ -22,14 +22,14 @@ using llvm::RecordKeeper;
 
 /// Emit the documentation for the given pass.
 static void emitDoc(const Pass &pass, raw_ostream &os) {
-  os << llvm::formatv("### `-{0}`\n", pass.getArgument());
+  os << llvm::formatv("\n### `-{0}`\n", pass.getArgument());
   emitSummary(pass.getSummary(), os);
   emitDescription(pass.getDescription(), os);
 
   // Handle the options of the pass.
   ArrayRef<PassOption> options = pass.getOptions();
   if (!options.empty()) {
-    os << "\n#### Options\n```\n";
+    os << "\n#### Options\n\n```\n";
     size_t longestOption = 0;
     for (const PassOption &option : options)
       longestOption = std::max(option.getArgument().size(), longestOption);
@@ -44,7 +44,7 @@ static void emitDoc(const Pass &pass, raw_ostream &os) {
   // Handle the statistics of the pass.
   ArrayRef<PassStatistic> stats = pass.getStatistics();
   if (!stats.empty()) {
-    os << "\n#### Statistics\n```\n";
+    os << "\n#### Statistics\n\n```\n";
     size_t longestStat = 0;
     for (const PassStatistic &stat : stats)
       longestStat = std::max(stat.getName().size(), longestStat);

mlir/tools/mlir-tblgen/OpDocGen.cpp

mlir/test/mlir-tblgen/gen-dialect-doc.td

GleasonK · 2025-02-06T18:13:18Z

Rebased which cleared CI issues, ready for re-review/merge if LGTY!

jpienaar

Thanks!

jpienaar

Thanks!

A few changes to doc generation: - All summaries are in italics. - In general each optional block starts and ends with a newline. - All table elements are enclosed in `|`'s - Overall reduce the number of >2newlines in a row Rationale for this change is that our markdown to docs generator requires a newline before all headers, otherwise it gets inlined into the line before it, see `### sdy-op-priority-propagate` in the image below. <img width="883" alt="image" src="https://github.com/user-attachments/assets/b795c424-cecb-48df-abbe-aee2030f4491" /> That said overall I feel this formatting is more consistent now, here's a before and after: - Dialect documentation diff: https://www.diffchecker.com/OVMHoXeL/ - Pass documentation diff: https://www.diffchecker.com/XEJRmW3k/

GleasonK added 2 commits December 13, 2024 19:21

[MLIR] Make generated markdown doc more consistent

2f027e4

Add tests

f04be09

llvmbot added mlir:core MLIR Core Infrastructure mlir labels Dec 13, 2024

GleasonK added the tablegen label Dec 13, 2024

GleasonK requested a review from jpienaar December 13, 2024 21:12

Add Pass.td dep to bazel overlay

2985880

GleasonK requested review from rupprecht and keith as code owners December 13, 2024 21:19

llvmbot added the bazel "Peripheral" support tier build system: utils/bazel label Dec 13, 2024

joker-eph reviewed Dec 14, 2024

View reviewed changes

mlir/tools/mlir-tblgen/OpDocGen.cpp Show resolved Hide resolved

joker-eph reviewed Dec 14, 2024

View reviewed changes

mlir/test/mlir-tblgen/gen-dialect-doc.td Outdated Show resolved Hide resolved

Add CHECK-NEXT and CHECK-EMPTY to ensure doc md structure

4b32a5b

GleasonK requested a review from joker-eph December 17, 2024 19:55

Merge branch 'llvm:main' into doc-gen

9edcdcb

GleasonK assigned joker-eph Feb 6, 2025

jpienaar approved these changes Feb 11, 2025

View reviewed changes

GleasonK merged commit 2cf6663 into llvm:main Feb 11, 2025
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[MLIR] Make generated markdown doc more consistent #119926

[MLIR] Make generated markdown doc more consistent #119926

Uh oh!

GleasonK commented Dec 13, 2024

Uh oh!

llvmbot commented Dec 13, 2024 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

GleasonK commented Feb 6, 2025

Uh oh!

jpienaar left a comment

Uh oh!

jpienaar left a comment

Uh oh!

Uh oh!

Uh oh!

[MLIR] Make generated markdown doc more consistent #119926

[MLIR] Make generated markdown doc more consistent #119926

Uh oh!

Conversation

GleasonK commented Dec 13, 2024

Uh oh!

llvmbot commented Dec 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

GleasonK commented Feb 6, 2025

Uh oh!

jpienaar left a comment

Choose a reason for hiding this comment

Uh oh!

jpienaar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

llvmbot commented Dec 13, 2024 •

edited

Loading