[PATCH] D73590: [mlir] Add a document detailing the design of the SymbolTable.

[Lei Zhang via Phabricator via llvm-commits]

antiagainst added inline comments.


================
Comment at: mlir/docs/SymbolsAndSymbolTables.md:27
+this, MLIR uses local pools for constant values as well as `Symbol` accesses for
+global values and variables. This document details the design of `Symbols`, what
+they are and how they fit into the system.
----------------
`Symbols` or `Symbol`?


================
Comment at: mlir/docs/SymbolsAndSymbolTables.md:30
+
+[TOC]
+
----------------
Isn't this too far down the page?


================
Comment at: mlir/docs/SymbolsAndSymbolTables.md:34
+refer to an operation symbolically with a name. This allows for referring to
+operations defined above regions that were defined as `IsolatedFromAbove` in a
+safe way.
----------------
We can also refer to symbols defined below regions right? Would be nice to be clear here. :)


================
Comment at: mlir/docs/SymbolsAndSymbolTables.md:56
+    -   This attribute defines the symbolic 'name' of the operation.
+*   Have an optional `StringAttr` attribute named
+    'SymbolTable::getVisibilityAttrName()'(`sym_visibility`)
----------------
It seems we should move this point to its own part given this part is talking about "*must* adhere to".


================
Comment at: mlir/docs/SymbolsAndSymbolTables.md:74
+and SSA values, or provide operations that materializes an SSA value from a
+symbol reference. Each have different trade offs depending on the situation. A
+function call may directly use a `SymbolRef` as the callee, whereas a reference
----------------
Each has?


================
Comment at: mlir/docs/SymbolsAndSymbolTables.md:114
+
+Described above are `Symbols`, which reside within a region of an operation
+defining a `SymbolTable`. A `SymbolTable` operation provides the container for
----------------
mehdi_amini wrote:
> The transitively nested aspect is ignored here again.
Nit: `Symbols` or `Symbol`s? I think typically we use the latter to mean plural form of some code. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73590/new/

https://reviews.llvm.org/D73590