[PATCH] D73491: [mlir][spirv] Add GroupNonUniform arithmetic operations.

[Mahesh Ravishankar via Phabricator via llvm-commits]

mravishankar marked an inline comment as done.
mravishankar added inline comments.


================
Comment at: mlir/include/mlir/Dialect/SPIRV/SPIRVNonUniformOps.td:334
+    non-uniform-imul-op ::= ssa-id `=` `spv.GroupNonUniformIMul` scope operation
+                            ssa-use ( `cluster_size` `(` ssa_use `)` )?
+                            `:` integer-scalar-vector-type
----------------
denis13 wrote:
> mravishankar wrote:
> > The SPIR-V spec says that the cluster_size must come from a constant. Do we want to just have an integer-literal here? We need to create the constant op while serialization too...
> @mravishankar thanks for review, can you please clarify a little bit,  the `cluster_size` is a SSA value and not a integral-literal, also check that `cluster_size` comes from constant at L662, and test for serialization. Do I miss something? Thanks.
Sorry about the serialization comment. The test indeed works with serialization.

In SPIR-V spec, the cluster_size is an SSA value, but it also says it has to come as a result of a constant instruction. One way to implement this is to accept an integer-literal in the op itself (so that you dont have an extra constant instruction generated to support this). But now that I think about this, the value could also come from a specialization constant. So its probably better to leave this as is, with the added advantage the the serialization will be auto-generated. So ignore this comment as a whole.


================
Comment at: mlir/lib/Dialect/SPIRV/SPIRVOps.cpp:631
+          << stringifyScope(static_cast<spirv::Scope>(
+                 groupOp->getAttrOfType<IntegerAttr>(kExecutionScopeAttrName)
+                     .getInt()))
----------------
One suggestion is to make this a templated method (templated on OpTy). Then you could use ".execution_scope()" and ".group_operation()" to get the attribute values


================
Comment at: mlir/lib/Dialect/SPIRV/SPIRVOps.cpp:646
+  spirv::Scope scope = static_cast<spirv::Scope>(
+      groupOp->getAttrOfType<IntegerAttr>(kExecutionScopeAttrName).getInt());
+  if (scope != spirv::Scope::Workgroup && scope != spirv::Scope::Subgroup)
----------------
Same here. Maybe this could be templated on OpTy.

Please add comments (one-liner should be fine) as well to describe what this method is for.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73491