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

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini added inline comments.


================
Comment at: mlir/docs/SymbolsAndSymbolTables.md:6
+compiler is built around this nesting structure, including the processing of
+operations within the [pass manager](WritingAPass.md#pass-manager). One nice
+aspect of MLIR is that it is able to process operations in parallel, utilizing
----------------
`nice` does not seems "nice" here ;)

What about "One advantage of the MLIR design is that... "?


================
Comment at: mlir/docs/SymbolsAndSymbolTables.md:39
+
+A `Symbol` is a named operation that resides within a region that defines a
+[`SymbolTable`](#symbol-table). The name of a symbol *must* be unique within the
----------------
`resides within a region that defines a SymbolTable` isn't accurate I believe because of the transitivity aspect.


================
Comment at: mlir/docs/SymbolsAndSymbolTables.md:41
+[`SymbolTable`](#symbol-table). The name of a symbol *must* be unique within the
+parent `SymbolTable`. This name is semantically similarly to an SSA result
+value, and may be referred to by other operations to provide a symbolic link, or
----------------
```
within the closest parent operation defining a `SymbolTable`
```


================
Comment at: mlir/docs/SymbolsAndSymbolTables.md:73
+is that dialects may need to ensure that their operations support `SymbolRefs`
+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
----------------
The part about "dialects may need to ensure that their operations support `SymbolRefs` and SSA values," isn't clear to me.


================
Comment at: mlir/docs/SymbolsAndSymbolTables.md:87
+
+As described above, `Symbols` act as an auxiliary way of defining uses of
+operations to the traditional SSA use-list. As such, it is imperative to provide
----------------
`SymbolRefs` instead of `Symbols` here (since this is about defining `uses`)


================
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
----------------
The transitively nested aspect is ignored here again.


================
Comment at: mlir/docs/SymbolsAndSymbolTables.md:145
+module @public_module {
+  // This function can be accessed by 'live.user'
+  func @nested_function() attributes { sym_visibility = "nested" }
----------------
I would add " but cannot be referenced externally: all the uses are in the parent regions"


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73590