[PATCH] D72116: [mlir][spirv] Update SPIR-V docs with information about utilities to convert to SPIR-V

[Lei Zhang via Phabricator via llvm-commits]

antiagainst requested changes to this revision.
antiagainst added a comment.
This revision now requires changes to proceed.

Nice! Just a few nits.



================
Comment at: mlir/docs/Dialects/SPIR-V.md:849
+targeting SPIR-V dialect using the Dialect Conversion framework, two utility
+class are provided.
+
----------------
s/class/classes/


================
Comment at: mlir/docs/Dialects/SPIR-V.md:851
+
+(**Note** : While SPIR-V has some [validation rules][SpirvShaderValidation],
+additional rules are imposed by [Vulkan shaders][VulkanSpirv]. The lowering
----------------
While SPIR-V has general validation rules, additional rules are imposed by the Vulkan execution environment. The lowering ... implements both sets of requirements.


================
Comment at: mlir/docs/Dialects/SPIR-V.md:879
+
+A base class that can be used to specify the patterns used for implementing the
+lowering. For now this only provides derived classes access to an instance of
----------------
`SPIRVOpLowering` is a base class ... can be used to define the patterns for ...


================
Comment at: mlir/docs/Dialects/SPIR-V.md:888
+In SPIR-V dialect, builtins are represented using `spv.globalVariable`s, with
+`spv._address_of` used to get a handle to the builtin as an SSA value.  The
+method `mlir::spirv::getBuiltinVariableValue` returns an SSA value, and creates
----------------
with `spv._address_of` to get ...


================
Comment at: mlir/docs/Dialects/SPIR-V.md:889
+`spv._address_of` used to get a handle to the builtin as an SSA value.  The
+method `mlir::spirv::getBuiltinVariableValue` returns an SSA value, and creates
+a `spv.globalVariable` for the builtin in the current `spv.module` if it does
----------------
The method ... creates a `spv.globalVariable` ..., and then returns such a SSA value generated from `spv._addresss_of`.


================
Comment at: mlir/docs/Dialects/SPIR-V.md:900
+
+#### Setting shader interface
+
----------------
Move this section as the first one? It's the most important one and sets the background for others.


================
Comment at: mlir/docs/Dialects/SPIR-V.md:921
+    operations conversions are implemented.
+*   [GPU Dialect][MlirGpuDialect] : Module with the attribute
+    `gpu.kernel_module` is converted to a `spv.module`. Functions within this
----------------
A module ...


================
Comment at: mlir/docs/Dialects/SPIR-V.md:923
+    `gpu.kernel_module` is converted to a `spv.module`. Functions within this
+    module with the attribute `gpu.kernel` is lowered as an entry function.
 
----------------
A function ... 


================
Comment at: mlir/docs/Dialects/SPIR-V.md:1160
 [GreedyPatternRewriter]: https://github.com/llvm/llvm-project/blob/master/mlir/lib/Transforms/Utils/GreedyPatternRewriteDriver.cpp
+[MlirDialectConversionTypeConversion]: https://github.com/llvm/llvm-project/blob/master/mlir/docs/DialectConversion.md#type-converter
+[MlirDialectConversionRewritePattern]: https://github.com/llvm/llvm-project/blob/master/mlir/docs/DialectConversion.md#conversion-patterns
----------------
Use https://mlir.llvm.org/docs/DialectConversion/ for these links


================
Comment at: mlir/docs/Dialects/SPIR-V.md:1163
+[MlirDialectConversionSignatureConversion]: https://github.com/llvm/llvm-project/blob/master/mlir/docs/DialectConversion.md#region-signature-conversion
+[MlirIntegerType]: https://github.com/llvm/llvm-project/blob/master/mlir/docs/LangRef.md#integer-type
+[MlirFloatType]: https://github.com/llvm/llvm-project/blob/master/mlir/docs/LangRef.md#floating-point-types
----------------
Similarly use https://mlir.llvm.org/docs/LangRef/ here and other markdown files


================
Comment at: mlir/docs/Dialects/SPIR-V.md:1192
 [CustomTypeAttrTutorial]: https://mlir.llvm.org/docs/DefiningAttributesAndTypes/
+[VulkanSpirv]: https://www.khronos.org/registry/vulkan/specs/1.1-extensions/html/vkspec.html#spirvenv
+[VulkanShaderInterface]: https://renderdoc.org/vkspec_chunked/chap14.html#interfaces-resources
----------------
Can we also point to the chunked doc here? The monolithic one is super slow to load.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72116