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