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

[River Riddle via Phabricator via cfe-commits]

rriddle added inline comments.


================
Comment at: mlir/include/mlir/Dialect/SPIRV/SPIRVControlFlowOps.td:463
+        Location loc, Value condition,
+        llvm::function_ref<void(OpBuilder *builder)> thenBody,
+        OpBuilder *builder);
----------------
Drop the llvm::.


================
Comment at: mlir/lib/Conversion/LinalgToSPIRV/LinalgToSPIRV.cpp:32
+
+  return llvm::all_of(op->getOperands(), isOfMemrefType) &&
+         llvm::all_of(op->getResults(), isOfMemrefType);
----------------
nit: You can use getOperandTypes() && getResultTypes() respectively.


================
Comment at: mlir/lib/Conversion/LinalgToSPIRV/LinalgToSPIRV.cpp:38
+static bool isLinalgSingleReductionIterator(ArrayAttr iterators) {
+  if (iterators.getValue().size() != 1)
+    return false;
----------------
Can you use size() directly on the ArrayAttr?


================
Comment at: mlir/lib/Conversion/LinalgToSPIRV/LinalgToSPIRV.cpp:66
+};
+}
+
----------------
nit: Missing end namespace comment.


================
Comment at: mlir/lib/Conversion/LinalgToSPIRV/LinalgToSPIRV.cpp:80
+  auto &region = op.region();
+  if (region.empty() || !has_single_element(region.getBlocks()))
+    return BinaryOpKind::Unknown;
----------------
You should be able to remove the .empty() call as well as the .getBlocks()


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73437