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 &region = 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

Reply via email to