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

[Nicolas Vasilache via Phabricator via cfe-commits]

nicolasvasilache accepted this revision.
nicolasvasilache added a comment.
This revision is now accepted and ready to land.
Herald added a subscriber: Joonsoo.

Thanks Lei, this looks great, glad to see you pushing on this front!



================
Comment at: mlir/lib/Conversion/LinalgToSPIRV/LinalgToSPIRV.cpp:125
+
+PatternMatchResult SingleWorkgroupReduction::matchAndRewrite(
+    linalg::GenericOp genericOp, ArrayRef<Value> operands,
----------------
antiagainst wrote:
> 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. 
Let's sync up over video for this, this is a longer discussion and touches phase ordering issues, local patterns etc.
If you want some of the deeper details please look at the rationale doc: https://mlir.llvm.org/docs/Dialects/Linalg/ (which will soon move to https://mlir.llvm.org/docs/Dialects/LinalgRationale/).

In the meantime, this is not a blocker but it would be great to sync up in particular on the implementation on those ideas today in DRR.


================
Comment at: mlir/lib/Conversion/LinalgToSPIRV/LinalgToSPIRV.cpp:156
+
+  auto inputMap = genericOp.indexing_maps().getValue()[0].cast<AffineMapAttr>();
+  auto outputMap =
----------------
antiagainst wrote:
> 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. :) 
SG, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73437