[PATCH] D73437: [mlir][spirv] Convert linalg.generic for reduction to SPIR-V ops

[Mahesh Ravishankar via Phabricator via llvm-commits]

mravishankar requested changes to this revision.
mravishankar added inline comments.
This revision now requires changes to proceed.


================
Comment at: mlir/lib/Conversion/LinalgToSPIRV/LinalgToSPIRV.cpp:36
+
+/// Returns true if the given Linalg `interators` is one reduction.
+static bool isLinalgSingleReductionIterator(ArrayAttr iterators) {
----------------
Nit : s/interators/iterators/


================
Comment at: mlir/lib/Conversion/LinalgToSPIRV/LinalgToSPIRV.cpp:73
+
+  using mlir::matchers::m_Val;
+  auto a = m_Val(block.getArgument(0));
----------------
I feel like this is not required. If the linalg.generic operation says this is a reduction, we should not be checking the region to verify that it is. linalg as a contract is guaranteeing this is a reduction.


================
Comment at: mlir/lib/Conversion/LinalgToSPIRV/LinalgToSPIRV.cpp:182
+  // Perform the group reduction operation.
+  Value groupOperation = rewriter.create<spirv::GroupNonUniformIAddOp>(
+      loc, originalInputType.getElementType(), spirv::Scope::Subgroup,
----------------
This is hard-wiring to IAdd. We probably need to structure this differently. We need to have a pattern to lower the linalg.generic with reduction iterator into the kernel generated here, and then lower the operations within the region separately.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73437