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 ®ion = 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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
