[mlir][docgen] Add ops source link

[rikhuijzer]

This patch suggests to change two things. Firstly, it adds a source link above the generated operations docs (above the emitOpDoc calls). This link will point directly to the source TableGen file for the group of operations. For example, for the current amdgpu page, the link will add a source link below the "Operation definition" heading pointing to mlir/include/mlir/Dialect/AMDGPU/IR/AMDGPU.td. The link is wrapped in a "op-definitions-source-link" class which could allow for custom styling, but it also looks reasonable without custom styling I think:

Secondly, this patch simplifies the header names such as "Operation definition" and "Attribute definition" to "Operations" and "Attributes" respectively. This is in line with manually defined subheadings on pages such as the one for the vector dialect.

[llvmbot]

@llvm/pr-subscribers-mlir-core

@llvm/pr-subscribers-mlir

Author: Rik Huijzer (rikhuijzer)

Changes

This patch suggests to change two things. Firstly, it adds a source link above the generated operations docs (above the emitOpDoc calls). This link will point directly to the source TableGen file for the group of operations. For example, for the current amdgpu page, the link will add a source link below the "Operation definition" heading pointing to mlir/include/mlir/Dialect/AMDGPU/IR/AMDGPU.td. The link is wrapped in a "op-definitions-source-link" class which could allow for custom styling, but it also looks reasonable without custom styling I think:

Secondly, this patch simplifies the header names such as "Operation definition" and "Attribute definition" to "Operations" and "Attributes" respectively. This is in line with manually defined subheadings on pages such as the one for the vector dialect.

---

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

1 Files Affected:

• (modified) mlir/tools/mlir-tblgen/OpDocGen.cpp (+26-10)

diff --git a/mlir/tools/mlir-tblgen/OpDocGen.cpp b/mlir/tools/mlir-tblgen/OpDocGen.cpp
index 773ad6ec198b957..cac1131c0369a9c 100644
--- a/mlir/tools/mlir-tblgen/OpDocGen.cpp
+++ b/mlir/tools/mlir-tblgen/OpDocGen.cpp
@@ -265,10 +265,24 @@ static void emitOpDoc(const Operator &op, raw_ostream &os) {
   os << "\n";
 }
 
+static void emitSourceLink(StringRef inputFilename, raw_ostream &os) {
+  size_t pathBegin = inputFilename.find("mlir/include/mlir/");
+  if (pathBegin == StringRef::npos)
+    return;
+
+  StringRef inputFromMlirInclude = inputFilename.substr(pathBegin);
+
+  os << "<span class=\"op-definitions-source-link\">\n";
+  os << "  <a href=\"https://github.com/llvm/llvm-project/blob/main/"
+     << inputFromMlirInclude << "\">source</a>\n";
+  os << "</span>\n\n";
+}
+
 static void emitOpDoc(const RecordKeeper &recordKeeper, raw_ostream &os) {
   auto opDefs = getRequestedOpDefinitions(recordKeeper);
 
   os << "<!-- Autogenerated by mlir-tblgen; don't manually edit -->\n";
+  emitSourceLink(recordKeeper.getInputFilename(), os);
   for (const llvm::Record *opDef : opDefs)
     emitOpDoc(Operator(opDef), os);
 }
@@ -392,12 +406,13 @@ static void maybeNest(bool nest, llvm::function_ref<void(raw_ostream &os)> fn,
   }
 }
 
-static void emitBlock(ArrayRef<Attribute> attributes,
+static void emitBlock(ArrayRef<Attribute> attributes, StringRef inputFilename,
                       ArrayRef<AttrDef> attrDefs, ArrayRef<OpDocGroup> ops,
                       ArrayRef<Type> types, ArrayRef<TypeDef> typeDefs,
                       raw_ostream &os) {
   if (!ops.empty()) {
-    os << "## Operation definition\n\n";
+    os << "## Operations\n\n";
+    emitSourceLink(inputFilename, os);
     for (const OpDocGroup &grouping : ops) {
       bool nested = !grouping.summary.empty();
       maybeNest(
@@ -417,32 +432,32 @@ static void emitBlock(ArrayRef<Attribute> attributes,
   }
 
   if (!attributes.empty()) {
-    os << "## Attribute constraint definition\n\n";
+    os << "## Attribute constraints\n\n";
     for (const Attribute &attr : attributes)
       emitAttrDoc(attr, os);
   }
 
   if (!attrDefs.empty()) {
-    os << "## Attribute definition\n\n";
+    os << "## Attributes\n\n";
     for (const AttrDef &def : attrDefs)
       emitAttrOrTypeDefDoc(def, os);
   }
 
   // TODO: Add link between use and def for types
   if (!types.empty()) {
-    os << "## Type constraint definition\n\n";
+    os << "## Type constraints\n\n";
     for (const Type &type : types)
       emitTypeDoc(type, os);
   }
 
   if (!typeDefs.empty()) {
-    os << "## Type definition\n\n";
+    os << "## Types\n\n";
     for (const TypeDef &def : typeDefs)
       emitAttrOrTypeDefDoc(def, os);
   }
 }
 
-static void emitDialectDoc(const Dialect &dialect,
+static void emitDialectDoc(const Dialect &dialect, StringRef inputFilename,
                            ArrayRef<Attribute> attributes,
                            ArrayRef<AttrDef> attrDefs, ArrayRef<OpDocGroup> ops,
                            ArrayRef<Type> types, ArrayRef<TypeDef> typeDefs,
@@ -456,7 +471,7 @@ static void emitDialectDoc(const Dialect &dialect,
   if (!r.match(dialect.getDescription()))
     os << "[TOC]\n\n";
 
-  emitBlock(attributes, attrDefs, ops, types, typeDefs, os);
+  emitBlock(attributes, inputFilename, attrDefs, ops, types, typeDefs, os);
 }
 
 static bool emitDialectDoc(const RecordKeeper &recordKeeper, raw_ostream &os) {
@@ -536,8 +551,9 @@ static bool emitDialectDoc(const RecordKeeper &recordKeeper, raw_ostream &os) {
             });
 
   os << "<!-- Autogenerated by mlir-tblgen; don't manually edit -->\n";
-  emitDialectDoc(*dialect, dialectAttrs, dialectAttrDefs, dialectOps,
-                 dialectTypes, dialectTypeDefs, os);
+  emitDialectDoc(*dialect, recordKeeper.getInputFilename(), dialectAttrs,
+                 dialectAttrDefs, dialectOps, dialectTypes, dialectTypeDefs,
+                 os);
   return false;
 }
 

[joker-eph]

Is HTML just valid in Markdown like that?

Can we use markdown syntax for the link?

[rikhuijzer]

Is HTML just valid in Markdown like that?

Yes. I've generated the code locally and it works there. By default this is disabled in Hugo, but MLIR has enabled raw HTML by setting goldmark unsafe (https://github.com/llvm/mlir-www/blob/c0925992314c8ac91be62adc8f1d9bddfd05098a/website/config.toml#L109-L113). The main attack that the "safe" mode defends against is attacks who insert arbitrary scripts inside the source files (but it doesn't protect against malicious Markdown links).

Can we use markdown syntax for the link?

Yes that's possible. I've just locally changed the lines to

  os << "[source](https://github.com/llvm/llvm-project/blob/main/"
     << inputFromMlirInclude << ")\n\n";

This works, but introduces extra spacing:

because Hugo wraps the link inside paragraph <p> tags, whereas I wrapped it in <span> tags which have no spacing by default.

I don't mind much. Let me know what you prefer and I'll put it in 👍

[joker-eph]

Not everyone downstream is using Hugo, so I rather not be to hung up to that.

[rikhuijzer]

Implemented in 62c8467

[rikhuijzer]

I just see that OpDocGen.cpp generates more plain HTML by the way, the attributes are generated as a HTML <table>:

llvm-project/mlir/tools/mlir-tblgen/OpDocGen.cpp

Lines 211 to 235 in 47df664

	os << "<table>\n";
	// Header.
	os << "<tr><th>Attribute</th><th>MLIR Type</th><th>Description</th></tr>\n";
	for (const auto &it : op.getAttributes()) {
	StringRef storageType = it.attr.getStorageType();
	// Name and storage type.
	os << "<tr>";
	os << "<td><code>" << it.name << "</code></td><td>" << storageType
	<< "</td><td>";
	StringRef description = resolveAttrDescription(it.attr);
	if (allowHugoSpecificFeatures && !description.empty()) {
	// Expandable description.
	// This appears as just the summary, but when clicked shows the full
	// description.
	os << "<details>"
	<< "<summary>" << it.attr.getSummary() << "</summary>"
	<< "{{% markdown %}}" << description << "{{% /markdown %}}"
	<< "</details>";
	} else {
	// Fallback: Single-line summary.
	os << it.attr.getSummary();
	}
	os << "</td></tr>\n";
	}
	os << "</table>\n";