This patch implements the conversion rule for operation introduced in https://reviews.llvm.org/D131200.
Also contains integration test for correctness
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorConversion.cpp | ||
---|---|---|
158 | looks like something is missing after "from an already converted ...." | |
167 | You use loc only once, so just inline op->getLoc() here | |
170 | same here, already converted .... | |
179 | You can use a bit more comments here, saying you first fill the sizes array from sparse/dense source, and then adjust it for the concatenation | |
292 | no line break in such a small method |
Address comments
mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorConversion.cpp | ||
---|---|---|
158 | I actually did not change it. Does the current version look more clear to you? |
last few nits
mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorConversion.cpp | ||
---|---|---|
158 | Yeah, you are right. Something strange happened to the comment along the way, or perhaps it was never right. One last request, since we have several "sizesFrom" (plural), it feels like this "sizeFrom" (singular) should be placed at L119, i.e. singular before plurals, just so that perhaps in the future we can share more code | |
593 | Also add a comment what this method does, before the TODOs ;-) | |
641 | period or : at end | |
666 | period at end | |
676 | period at end | |
1229 | 1st, 2nd -> (1) (2) for readability | |
1232 | These comments use a lot of vertical space. Since you already spelled out a loop nest in the loop generator above for i,j,k // dense_b coo->add(adjustForOffset(i,j,k), b[i,j,k]) for elem // sparse_c coo->add(adjustForOffset(elem.indices), elem.value) or something like that That way, the actual code (L1257!) is a little "closer" :-) |
looks like something is missing after "from an already converted ...."