This is an archive of the discontinued LLVM Phabricator instance.

[mlir][sparse] support coiteration over sparse tensor slices
ClosedPublic

Authored by Peiming on Dec 28 2022, 11:53 AM.

Diff Detail

Event Timeline

Peiming created this revision.Dec 28 2022, 11:53 AM
Herald added a project: Restricted Project. · View Herald Transcript
Peiming requested review of this revision.Dec 28 2022, 11:53 AM
Peiming updated this revision to Diff 485542.Dec 28 2022, 1:15 PM

update comments.

Peiming updated this revision to Diff 485554.Dec 28 2022, 4:16 PM

more test cases.

aartbik added inline comments.Feb 7 2023, 5:49 PM
mlir/lib/Dialect/SparseTensor/Transforms/LoopEmitter.cpp
94

First, second, third (from previous)

572

You use s and non-s forms (Skip, Generates, Finds)

Typically, we use s form in top comment, and non-s form in code doc
(but at the very least, pick one style consistently)

mlir/lib/Dialect/SparseTensor/Transforms/LoopEmitter.h
185

user's or users'

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

is the attr test needed since already done in if?
oh wait, perhaps due to the ;

in that case, can you just break out first and second if for readability?

1071

// break on

either on same line as break (preferred)

or otherwise in right style

// Break on ... .

mlir/test/Dialect/SparseTensor/sparse_1d.mlir
1302–1303

should we make the load a CHECK-DAG if order may change (often) over time?

mlir/test/Integration/Dialect/SparseTensor/CPU/sparse_matmul_slice.mlir
48

commend needs refining on format

60

in DCSR is no longer valid in the comment

Peiming updated this revision to Diff 495907.Feb 8 2023, 11:46 AM
Peiming marked 7 inline comments as done.

address comments.

mlir/test/Dialect/SparseTensor/sparse_1d.mlir
1302–1303

Yeah, good idea

aartbik accepted this revision.Feb 14 2023, 6:08 PM
aartbik added inline comments.
mlir/lib/Dialect/SparseTensor/Transforms/Sparsification.cpp
1069

period at end

1071

Oh, I was actually thinking of

if (auto attr = ....)

if (attr == ....)

but up to you what style you prefer

This revision is now accepted and ready to land.Feb 14 2023, 6:08 PM
Peiming updated this revision to Diff 497822.Feb 15 2023, 3:27 PM
Peiming marked an inline comment as done.

address comment.

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

Yeah, you are right! no need to test attr != nullptr as they are pointer comparison afterall!

Peiming updated this revision to Diff 497832.Feb 15 2023, 3:50 PM
Peiming marked an inline comment as done.

fix memory leakage in test case

This revision was landed with ongoing or failed builds.Feb 15 2023, 3:52 PM
This revision was automatically updated to reflect the committed changes.