Page MenuHomePhabricator

[MLIR][Sparse] Refactor lattice code into its own file
ClosedPublic

Authored by gussmith23 on Jun 22 2021, 4:34 PM.

Details

Summary

Moves iteration lattice/merger code into new SparseTensor/Utils directory. A follow-up CL will add lattice/merger unit tests.

Diff Detail

Event Timeline

gussmith23 created this revision.Jun 22 2021, 4:34 PM
gussmith23 requested review of this revision.Jun 22 2021, 4:34 PM
gussmith23 retitled this revision from Refactor lattice code into its own file to [MLIR][Sparse] Refactor lattice code into its own file.Jun 22 2021, 4:35 PM
aartbik added inline comments.Jun 23 2021, 2:39 PM
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

aartbik added inline comments.Jun 24 2021, 12:05 AM
mlir/include/mlir/Dialect/SparseTensor/Utils/Lattice.h
13 ↗(On Diff #353819)

TRANSFORMS -> UTILS

aartbik added inline comments.Jun 24 2021, 9:39 AM
mlir/lib/Dialect/SparseTensor/Utils/CMakeLists.txt
9

the utils alone does not depend on linalg, right?

aartbik added inline comments.Jun 24 2021, 11:39 AM
mlir/include/mlir/Dialect/SparseTensor/Utils/Lattice.h
16 ↗(On Diff #353819)

why are we pulling this in?

Updates from Aart reviews on Critique

gussmith23 marked 3 inline comments as done.

Address Aart's comments

gussmith23 added inline comments.Jun 24 2021, 1:25 PM
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.

aartbik added inline comments.Jun 24 2021, 1:30 PM
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.
Agreed?

Address Aart's comments

Address Aart's comments

aartbik added inline comments.Jun 24 2021, 2:59 PM
mlir/lib/Dialect/SparseTensor/Transforms/Sparsification.cpp
44–49

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).

Re-introduce includes, cmake fix

gussmith23 marked an inline comment as done.Jun 24 2021, 3:23 PM
gussmith23 added inline comments.
mlir/lib/Dialect/SparseTensor/Transforms/Sparsification.cpp
44–49

Brought them back!

aartbik accepted this revision.Jun 24 2021, 4:00 PM
This revision is now accepted and ready to land.Jun 24 2021, 4:00 PM
This revision was automatically updated to reflect the committed changes.
gussmith23 marked an inline comment as done.