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

Lei Zhang via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Jan 31 06:12:50 PST 2020


antiagainst marked 4 inline comments as done.
antiagainst added inline comments.


================
Comment at: mlir/lib/Conversion/LinalgToSPIRV/LinalgToSPIRV.cpp:125
+
+PatternMatchResult SingleWorkgroupReduction::matchAndRewrite(
+    linalg::GenericOp genericOp, ArrayRef<Value> operands,
----------------
nicolasvasilache wrote:
> 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.
Awesome, thanks! I've opened https://mlir.llvm.org/docs/Dialects/Linalg/ in a tab but haven't read it through yet. ;-P Will do now. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73437





More information about the llvm-commits mailing list