Details
- Reviewers
aartbik nicolasvasilache bixia wrengr anlunx
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
mlir/lib/Dialect/SparseTensor/Transforms/Sparsification.cpp | ||
---|---|---|
173–174 | This is (probably) a typo, but I am not sure, it does not affect the patch actually. Let me know if you want to keep it or not. |
mlir/lib/Dialect/SparseTensor/Transforms/Sparsification.cpp | ||
---|---|---|
473–477 | The patch is updated per offline discussion (by only picking one AffineDimExpr from compound affines to establish the order instead of adding order constraints between every pair of their sub-AffineDimExprs) |
mlir/test/Integration/Dialect/SparseTensor/CPU/sparse_conv_3d_ndhwc_dhwcf.mlir | ||
---|---|---|
7 | we can now also support codegen path for sparse convolution. |
mlir/lib/Dialect/SparseTensor/Transforms/Sparsification.cpp | ||
---|---|---|
173–174 | Despite that we had an offline discussion on this, I still think it should be getNumDims() after taking more time understanding resolveCycles. Also, I believe currently it failed to handle cases when NumDims != NumResults, e.g., AffineMap<(d0, d1, d2, d3) -> (d0, d1)> |
mlir/lib/Dialect/SparseTensor/Transforms/Sparsification.cpp | ||
---|---|---|
173–174 | Yes, you are absolutely right. I thought we discussed num inputs vs num dims (since they should be the same), but num results is of course wrong! the topsort has the size of the "parallel" index space. So either Dims or Inputs should be used. |
I made several attempts to review this as is, but there are too many changes at different places at once for me to do a thorough good job.
Let's discuss offline on how to split this into smaller, incremental steps (even if some merely improve analysis without codegen yet), so I can better track what is changing.
mlir/include/mlir/Dialect/SparseTensor/Utils/Merger.h | ||
---|---|---|
151 | comment needs to be expanded also, you can probably separate the merger changes (and some unit testing) | |
241 | just numNativeLoops? (given that you do that for all others too, it is local enough that this does not go out of sync) | |
mlir/lib/Dialect/SparseTensor/Transforms/CodegenUtils.cpp | ||
472 | was this already set right? | |
mlir/lib/Dialect/SparseTensor/Transforms/CodegenUtils.h | ||
527 | This is really just the inverse topsort, right? It makes sense you need this for affine mapping, but the explanation right now seems to explain it in terms of the affine mapping, rather than its true meaning? |
mlir/lib/Dialect/SparseTensor/Transforms/CodegenUtils.cpp | ||
---|---|---|
472 | Yes, because filter loop will look like for() if (idx == affine) // user-generated so, we need to generate yield in the ifOp instead of forOp But maybe I should add a doc to indicate that it is the users' responsibility to ensure the insertion point is correct. |
comment needs to be expanded
also, you can probably separate the merger changes (and some unit testing)
in its own revision for easier reviewing