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