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