Moves iteration lattice/merger code into new SparseTensor/Utils directory. A follow-up CL will add lattice/merger unit tests.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
mlir/include/mlir/Dialect/SparseTensor/Utils/Lattice.h | ||
---|---|---|
69 ↗ | (On Diff #353819) | For classes we often keep the 1:1 with header file and class name, so perhaps (in a next CL) we should rename Merger to "Lattice"? What do you think The TensorExpr and LatPoint structs are smaller, and can easily live here too, but could perhaps even be nested inside Lattice at a future, date, but due to namespaces, there is less urgency to that. |
120 ↗ | (On Diff #353819) | you moved all code from header to cpp (good), but starting from here down, you keep it "inline". was this on purpose? some are small, but some also have loops in them, so just curious what your reasoning was |
mlir/include/mlir/Dialect/SparseTensor/Utils/Lattice.h | ||
---|---|---|
13 ↗ | (On Diff #353819) | TRANSFORMS -> UTILS |
mlir/lib/Dialect/SparseTensor/Utils/CMakeLists.txt | ||
---|---|---|
9 | the utils alone does not depend on linalg, right? |
mlir/include/mlir/Dialect/SparseTensor/Utils/Lattice.h | ||
---|---|---|
16 ↗ | (On Diff #353819) | why are we pulling this in? |
mlir/include/mlir/Dialect/SparseTensor/Utils/Lattice.h | ||
---|---|---|
69 ↗ | (On Diff #353819) | Hm, it depends -- does a Merger truly represent a single lattice? It seems to me like it actually represents the multiple lattices present in a single kernel, unless all of them collectively are actually referred to as a single lattice. There's probably a clearer name than "merger", probably involving "lattice"...could be as simple as "Lattices", or maybe "ExpressionLattices", or "LatticeSet"... |
120 ↗ | (On Diff #353819) | Should I move all impls to the cpp? My general thinking was just "keep short impls inline" because that's what I've seen others do. I can move the hasAnyDimOf impl over to Lattice.cpp, or I can just move all of them over! Let me know what you'd prefer. |
mlir/include/mlir/Dialect/SparseTensor/Utils/Lattice.h | ||
---|---|---|
69 ↗ | (On Diff #353819) | I am fine with original name, but was just thinking of the filename Lattice vs. class name Merger. But I don't want to overthink it either, since this does not seem to be a strict llvm style anyway. |
120 ↗ | (On Diff #353819) | Let's move hasAnyDimOf and onlyDenseDiff over, keep all others here. |
mlir/lib/Dialect/SparseTensor/Transforms/Sparsification.cpp | ||
---|---|---|
45–55 | One last question, we seem to remove more includes here then the new Merger.h brings back in. |
mlir/lib/Dialect/SparseTensor/Transforms/Sparsification.cpp | ||
---|---|---|
45–55 | Brought them back! |
One last question, we seem to remove more includes here then the new Merger.h brings back in.
Is this as a result of auto cleanup, since I expect some to be necessary (like linalg at least).