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

Thu Jan 30 06:22:15 PST 2020

antiagainst added inline comments.

================
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:
> mravishankar wrote:
> > antiagainst wrote:
> > > 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.
> > The way I see this structured is
> > 
> > 1) You check the "structure" of the linalg.generic op to see if it is a 1D reduction. You assume that the body of the generic op represents a binary operation that can be used for the reduction.
> > 2) You write a separate pattern that converts the operations of the body itself into the spirv::GroupNonUniform*Op.
> > 
> > The dialect conversion will first convert the generic op, and then it will attempt to convert the body of the op. The way this is strucutred, it can only handle straight-forward binary operations. THere could be more complicated operations in the region of a generic op that implements the reduction, which would be hard to incorporate in this approach.
> > 
> > With your updated changes, it is easy to extend to other ops, but I think a more general solution is needed where we are not constrained in handling just simple reductions. It might need some more careful design though, which I dont have a full picture of right now. So for now this OK, but please leave a TODO to say something like "handle more general reductions".
> Thought about this a little bit more. It will be a more sane strategy to not handle cases where the region is "inlined" into the generic op, but rather the region is specified as a function using the `fun` attribute of the `linalg.generic` op. Then we can split the lowering of the generic op and the lowering of the function that does the reduction separately.
> This too not needed for this CL. Should revisit this
Yeah, right now linalg.generic is an integrated entity; I'm not sure how we can easily convert part of it (the "structure") and then the inlined scalar region. I guess it's fine for now to overfit a bit until later we see concrete cases that we want to support, which will give us a more tangible problem to solve. 

================
Comment at: mlir/lib/Conversion/LinalgToSPIRV/LinalgToSPIRV.cpp:27
+/// types.
+static bool areAllValuesMemref(Operation *op) {
+  auto isOfMemrefType = [](Value val) {
----------------
nicolasvasilache wrote:
> Please use `LinalgOp::hasBufferSemantics` instead of rolling your own
Oh, didn't know we have that. Thanks!

================
Comment at: mlir/lib/Conversion/LinalgToSPIRV/LinalgToSPIRV.cpp:80
+  auto &region = op.region();
+  if (region.empty() || !has_single_element(region.getBlocks()))
+    return BinaryOpKind::Unknown;
----------------
rriddle wrote:
> You should be able to remove the .empty() call as well as the .getBlocks()
Nice, thanks!

================
Comment at: mlir/lib/Conversion/LinalgToSPIRV/LinalgToSPIRV.cpp:125
+
+PatternMatchResult SingleWorkgroupReduction::matchAndRewrite(
+    linalg::GenericOp genericOp, ArrayRef<Value> operands,
----------------
nicolasvasilache wrote:
> 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. 
I feel I don't have all the backgrounds here so I'd appreciate some explanation as for why it's necessary to separate the match and rewrite. I think that's the original design of patterns (that we have `match` and `rewrite` separately) and then later we found it's too cumbersome to carry over a large matched state between them and then came up with `matchAndRewrite`? IIUC, every pattern is applied as a whole. I can understand that the match/rewrite side can be potentially reusable across patterns as components if they are exposed as helper functions, but that's more from a library organization and code reuse's perspective; I feel you've more reasons than that so I'd like to learn about them. :) 

Another question here is that I've matches that is generally useful to other patterns (checking whether this is a linalg doing reduction) and matches that is only useful to SPIR-V here (checking whether the workload can be fit in one local workgroup). How do you handle such cases?

For now I separated the check that whether this is a linalg doing reduction as a helper function. I haven't put it in some linalg file yet because currently it's too rigid and overfitting to a specific case. I plan to extend it and then later maybe we can move it to some linalg file. 

================
Comment at: mlir/lib/Conversion/LinalgToSPIRV/LinalgToSPIRV.cpp:156
+
+  auto inputMap = genericOp.indexing_maps().getValue()[0].cast<AffineMapAttr>();
+  auto outputMap =
----------------
nicolasvasilache wrote:
> 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. 
Right. I guess here we need some thinking about how to better design the API to make it generic and usable across different cases. I'm not so versed on that at the current moment. So putting your name in the TODO for now. :) 

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73437