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

[Lei Zhang via Phabricator via cfe-commits]

antiagainst marked 2 inline comments as done.
antiagainst added inline comments.
Herald added a reviewer: mclow.lists.


================
Comment at: mlir/lib/Conversion/LinalgToSPIRV/LinalgToSPIRV.cpp:27
+/// types.
+static inline bool areAllValuesMemref(Operation *op) {
+  auto isOfMemrefType = [](Value val) {
----------------
rriddle wrote:
> Why the inline keyword?
Added initially because this function was trivial enough. I guess right now it's better to let the compiler decide.


================
Comment at: mlir/lib/Conversion/LinalgToSPIRV/LinalgToSPIRV.cpp:60
+  auto &region = op.region();
+  if (region.empty() || region.getBlocks().size() != 1)
+    return false;
----------------
rriddle wrote:
> Don't use size, it is O(N). Use 'has_single_element' instead.
Oh, good to know! Thanks!


================
Comment at: mlir/lib/Conversion/LinalgToSPIRV/LinalgToSPIRV.cpp:70
+  auto &ops = block.getOperations();
+  if (ops.size() != 2)
+    return false;
----------------
rriddle wrote:
> Same here, size() is O(N): `!has_single_element(block.without_terminator())`
Nice, thanks!


================
Comment at: mlir/lib/Conversion/LinalgToSPIRV/LinalgToSPIRV.cpp:73
+
+  using mlir::matchers::m_Val;
+  auto a = m_Val(block.getArgument(0));
----------------
mravishankar wrote:
> 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.
I'm not sure I get your idea correctly; but this is checking we are doing the expected kind of reduction (`BinaryOp`). 


================
Comment at: mlir/lib/Conversion/LinalgToSPIRV/LinalgToSPIRV.cpp:103
+    linalg::GenericOp genericOp, ArrayRef<Value> operands,
+    ConversionPatternRewriter &rewriter) const {
+  Operation *op = genericOp.getOperation();
----------------
rriddle wrote:
> This function is huge, is there any way to split it up a bit?
Good point. Split out two helper functions.


================
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,
----------------
mravishankar wrote:
> 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.
It was intentional to only support IAdd. I've re-structured this a bit so it's extensible to other binary op kinds.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73437