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

[Lei Zhang via Phabricator via llvm-commits]

antiagainst marked an inline comment as done.
antiagainst 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
----------------
mravishankar wrote:
> 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.
Right. In SPIR-V constant instructions include both normal OpConstant* and specialization OpSpecConstant*, so normally we don't want to represent a constant operand onto the op itself as an attribute. We do it for cases like for scope and memory semantic <id>s because the validation rule explicitly said that "All <id> used for Scope and Memory Semantics must be of an OpConstant". Making scope and memory semantics as attributes indeed improves the IR readability and handling.  However, the specific validation rule is for `Shader` capability though; we need to relax this if, in the future, we support `Kernel` capability cases, or we need to design our own `IntegerAttr` that can encode the spec constant ID. At that time, maybe we can rethink here too whether we want to use that special `IntegerAttr`. It's a broader scope change though: we need modelling `EnumAttr` for it and all the stuff associated. Would prefer to defer until necessary. :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73491