[Mlir-commits] [mlir] [mlir][SPIR-V] Accept spec constants for GroupNonUniform cluster size (PR #214408)

[Igor Wodiany]

================
@@ -33,14 +33,16 @@ static LogicalResult verifyGroupNonUniformArithmeticOp(Operation *groupOp) {
                                 "'ClusteredReduce' group operation");
   if (groupOp->getNumOperands() > 1) {
     Operation *sizeOp = groupOp->getOperand(1).getDefiningOp();
-    int32_t clusterSize = 0;
 
-    // TODO: support specialization constant here.
-    if (failed(extractValueFromConstOp(sizeOp, clusterSize)))
+    // SPIR-V spec: "ClusterSize must come from a constant instruction",
+    // which includes both a normal constant and a specialization constant.
+    if (!isa_and_present<spirv::ConstantOp, spirv::ReferenceOfOp>(sizeOp))
       return groupOp->emitOpError(
           "cluster size operand must come from a constant op");
 
-    if (!llvm::isPowerOf2_32(clusterSize))
+    int32_t clusterSize = 0;
+    if (succeeded(extractValueFromConstOp(sizeOp, clusterSize)) &&
----------------
IgWod wrote:

I know this is already here, but I don't we should be calling `extractValueFromConstOp` at all in the verifier since it against MLIR guidelines:

> TLDR: only verify local aspects of an operation, in particular don’t follow def-use chains (don’t look at the producer of any operand or the user of any results).

If we want to do such verification here we should be using constant as an attribute mechanism: https://github.com/llvm/llvm-project/blob/9e36e5a44bb5ade74202b01246cab4231d202972/mlir/tools/mlir-tblgen/SPIRVUtilsGen.cpp#L498

https://github.com/llvm/llvm-project/pull/214408