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

[Nicolas Vasilache via Phabricator via llvm-commits]

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


================
Comment at: mlir/lib/Conversion/LinalgToSPIRV/LinalgToSPIRV.cpp:27
+/// types.
+static bool areAllValuesMemref(Operation *op) {
+  auto isOfMemrefType = [](Value val) {
----------------
Please use `LinalgOp::hasBufferSemantics` instead of rolling your own


================
Comment at: mlir/lib/Conversion/LinalgToSPIRV/LinalgToSPIRV.cpp:37
+/// Returns true if the given Linalg `iterators` is one reduction.
+static bool isLinalgSingleReductionIterator(ArrayAttr iterators) {
+  if (iterators.getValue().size() != 1)
----------------
Can we create a `singleReductionIterator()` somewhere in the existing utils and directly equality compare the `ArrayAttr iterators` against that?
I anticipate this type of things is soon going to become more and more useful, including from Tablegen. 


================
Comment at: mlir/lib/Conversion/LinalgToSPIRV/LinalgToSPIRV.cpp:78
+/// ```
+static BinaryOpKind getScalarBinaryOpKind(linalg::GenericOp op) {
+  auto &region = op.region();
----------------
Same thing here, we should move these to utils on the Linalg side and make reusable as things are soon going to blow up out of control otherwise.


================
Comment at: mlir/lib/Conversion/LinalgToSPIRV/LinalgToSPIRV.cpp:125
+
+PatternMatchResult SingleWorkgroupReduction::matchAndRewrite(
+    linalg::GenericOp genericOp, ArrayRef<Value> operands,
----------------
Please split this into a precondition and an apply that must succeed so we can use the precondition as a Constraint and the apply as a simple Rewrite.
I have found this split to be crucial to write custom fused passes with DRR.
I claim this is generally the way we should be writing transformations when we can, which is the case for Linalg ops. 


================
Comment at: mlir/lib/Conversion/LinalgToSPIRV/LinalgToSPIRV.cpp:156
+
+  auto inputMap = genericOp.indexing_maps().getValue()[0].cast<AffineMapAttr>();
+  auto outputMap =
----------------
I have similar comments re utils here but it will take a bit longer to put things in a reasonable form so please just add a TODO that this should use generic Linalg utils when available. 


================
Comment at: mlir/lib/Conversion/LinalgToSPIRV/LinalgToSPIRV.cpp:260
+
+void mlir::populateLinalgToSPIRVPatterns(MLIRContext *context,
+                                         SPIRVTypeConverter &typeConverter,
----------------
Thanks for doing this as a pattern, it will compose nicely thanks to all of @rriddle's great work!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73437