This is an archive of the discontinued LLVM Phabricator instance.

[mlir][sparse] remove vector support in sparsification
ClosedPublic

Authored by Peiming on Oct 18 2022, 9:41 AM.

Details

Summary

Sparse compiler used to generate vectorized code for sparse tensors computation, but it should really be delegated to other vectorization passes for better progressive lowering.

https://discourse.llvm.org/t/rfc-structured-codegen-beyond-rectangular-arrays/64707

Diff Detail

Event Timeline

Peiming created this revision.Oct 18 2022, 9:41 AM
Herald added a reviewer: aartbik. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
Peiming requested review of this revision.Oct 18 2022, 9:41 AM
Peiming retitled this revision from [mlir][sparse] remove vector support in sparsfication to [mlir][sparse] remove vector support in sparsification.Oct 18 2022, 10:08 AM
aartbik added inline comments.Oct 18 2022, 11:06 AM
mlir/lib/Dialect/SparseTensor/Transforms/Sparsification.cpp
899

note that this options can now also be removed from SparsificationOptions

mlir/test/Integration/Dialect/SparseTensor/CPU/sparse_cast.mlir
5–6

Please remove L7-L13 completely from all integration tests that do non-vector/vector run, since it will be easy to add back

mlir/test/Integration/Dialect/SparseTensor/python/test_SDDMM.py
142–143

remove the # part, we can easily add that back

Peiming updated this revision to Diff 468645.Oct 18 2022, 11:57 AM

address comments from Aart.

Peiming updated this revision to Diff 468648.Oct 18 2022, 12:00 PM
Peiming marked an inline comment as done.

address comments

Peiming updated this revision to Diff 468649.Oct 18 2022, 12:01 PM
Peiming marked 2 inline comments as done.

missing changes..

Peiming updated this revision to Diff 468650.Oct 18 2022, 12:03 PM

append missing changes..

aartbik added inline comments.Oct 18 2022, 2:09 PM
mlir/lib/Dialect/SparseTensor/Transforms/Sparsification.cpp
724–725

Note that the "For the scalar case" and the "Here too" comment is a bit outdated now.

mlir/test/Dialect/SparseTensor/sparse_parallel.mlir
3–11

let's remove this too and PAR1-4

mlir/test/Dialect/SparseTensor/sparse_vector.mlir
0–2

I would remove all the CHECK-VEC, and just keep the single CHECK now

mlir/test/Integration/Dialect/SparseTensor/CPU/sparse_out_simple.mlir
6–7

Remove L8-L15

mlir/test/Integration/Dialect/SparseTensor/CPU/sparse_sum.mlir
6–7

remove L8-L15

Peiming added inline comments.Oct 18 2022, 4:11 PM
mlir/test/Dialect/SparseTensor/sparse_parallel.mlir
3–11

This will be back later in D135927, so I think it is good to keep it here?

Peiming updated this revision to Diff 468736.Oct 18 2022, 4:19 PM
Peiming marked 4 inline comments as done.

remove unused check.

Peiming edited the summary of this revision. (Show Details)Oct 18 2022, 4:26 PM
wrengr added inline comments.Oct 18 2022, 4:29 PM
mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorPasses.cpp
66–68

Did you mean to remove the last line (setting enableRuntimeLibrary)?

mlir/lib/Dialect/SparseTensor/Transforms/Sparsification.cpp
754–755

Since you're just returning the ival immediately, should combine these two lines to just return codegen.loops[idx];

Peiming added inline comments.Oct 18 2022, 4:30 PM
mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorPasses.cpp
66–68

Nice catch! such an embarrassing mistake ;-(

Peiming updated this revision to Diff 468743.Oct 18 2022, 4:51 PM

remove useless dependency.

Peiming updated this revision to Diff 468744.Oct 18 2022, 4:54 PM

[mlir][sparse] remove enableRT flag from sparsification passes.

Peiming marked an inline comment as done.Oct 18 2022, 5:05 PM
Peiming added inline comments.
mlir/lib/Dialect/SparseTensor/Transforms/Sparsification.cpp
754–755

I will do the merge in the depended CL (D136185) instead to avoid some rebase conflict, is that okay?

Peiming marked an inline comment as done.Oct 18 2022, 5:05 PM
Peiming added inline comments.Oct 18 2022, 5:08 PM
mlir/lib/Dialect/SparseTensor/Transforms/Sparsification.cpp
754–755

I will do the merge in the depended CL (D136185) instead to avoid some rebase conflict, is that okay?

sorry I mean dependent CL

Peiming updated this revision to Diff 468753.Oct 18 2022, 5:23 PM

update BUILD file, remove unused headers.

I am a bit confused on the enableRuntimeLibrary change. Did you upload?

mlir/include/mlir/Dialect/SparseTensor/Transforms/Passes.td
127

is this gone too now?

mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorPasses.cpp
66–68

Peiming, the line is still gone, did you upload?

73

here too

aartbik added a comment.EditedOct 19 2022, 11:00 AM

I see it is now gone from SparsificationOptions, but not the SparseCompilerOptions. Please make a note in the description that it also cleans up this flag, then I am okay.

Peiming marked 3 inline comments as done.Oct 19 2022, 11:01 AM
Peiming added inline comments.
mlir/include/mlir/Dialect/SparseTensor/Transforms/Passes.td
127

Yes, See L48 @ SparseTensorPasses.cpp

aartbik accepted this revision.Oct 19 2022, 11:05 AM

Farewell, good SIMD friend!

mlir/lib/Dialect/SparseTensor/Transforms/Sparsification.cpp
905

Please keep this comment, but remove the "For the scalar case" and the "Here too" (since there is no longer a constrast with the vector code). But the comment itself is still valid

This revision is now accepted and ready to land.Oct 19 2022, 11:05 AM
Peiming marked 2 inline comments as done.Oct 19 2022, 11:10 AM
Peiming added inline comments.
mlir/lib/Dialect/SparseTensor/Transforms/Sparsification.cpp
905

See L725 on the right hand sides.

// Simply zero extends narrower indices into 64-bit values before casting to
// index without a performance penalty.
This revision was automatically updated to reflect the committed changes.
Peiming marked an inline comment as done.
mlir/test/Dialect/SparseTensor/sparse_vector.mlir