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

[River Riddle via Phabricator via llvm-commits]

rriddle added inline comments.


================
Comment at: mlir/include/mlir/Conversion/LinalgToSPIRV/LinalgToSPIRV.h:3
+//
+// Part of the MLIR Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
----------------
Should be LLVM Project now.


================
Comment at: mlir/lib/Conversion/LinalgToSPIRV/LinalgToSPIRV.cpp:27
+/// types.
+static inline bool areAllValuesMemref(Operation *op) {
+  auto isOfMemrefType = [](Value val) {
----------------
Why the inline keyword?


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


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


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


================
Comment at: mlir/lib/Conversion/LinalgToSPIRV/LinalgToSPIRVPass.cpp:42
+
+  if (failed(applyFullConversion(module, *target, patterns))) {
+    return signalPassFailure();
----------------
Drop the trivial brace.


================
Comment at: mlir/lib/Dialect/SPIRV/TargetAndABI.cpp:70
+    op = op->getParentOp();
+
+  if (!op)
----------------
nit: Drop the newline here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73437