This is an archive of the discontinued LLVM Phabricator instance.

[mlir][sparse] make foreach operation support sparse tensor slices.
ClosedPublic

Authored by Peiming on Dec 27 2022, 5:43 PM.

Diff Detail

Event Timeline

Peiming created this revision.Dec 27 2022, 5:43 PM
Herald added a project: Restricted Project. · View Herald Transcript
Peiming requested review of this revision.Dec 27 2022, 5:43 PM
Peiming updated this revision to Diff 485516.Dec 28 2022, 9:21 AM

clean up.

Peiming updated this revision to Diff 485517.Dec 28 2022, 9:36 AM

update comments.

bixia added inline comments.Jan 12 2023, 10:50 AM
mlir/lib/Dialect/SparseTensor/Transforms/LoopEmitter.cpp
61

This function looks more like fromSliceCoord to me, where "v" is the coordinate in the slice view. Am I right?

66

iv = slice_iv * stride + offset? I am not sure whether we should also add a period in this case?

72

Related to the above, this looks like toSliceCoord to me.

78

slice_iv = (iv - offset)/stride?

379

confirm?

mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorCodegen.cpp
962

Converter?

1022–1029

I don't see FileCheck test for this pattern, can we add such test?

mlir/test/Integration/Dialect/SparseTensor/CPU/sparse_foreach_slices.mlir
6

Do we really need this lib?

50–53

We should avoid having the second part repeat the first part, maybe even try to avoid having the same NZ values in two different positions.

Peiming updated this revision to Diff 488765.Jan 12 2023, 1:51 PM
Peiming marked 4 inline comments as done.

address comments.

mlir/lib/Dialect/SparseTensor/Transforms/LoopEmitter.cpp
61

Hmmm, yeah, updated.

379

No, I confirmed that it is folded.

But it does not really matter, I manually skip the check anyway in the following revision.

mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorCodegen.cpp
1022–1029

This is a nop right now (the check test is added in the dynamic slice revision, which actually implements it).

Peiming marked 3 inline comments as done.Jan 12 2023, 1:52 PM
aartbik added inline comments.Feb 7 2023, 10:47 AM
mlir/lib/Dialect/SparseTensor/IR/SparseTensorDialect.cpp
420

so what happens if you do "the right thing" here already?

mlir/lib/Dialect/SparseTensor/Transforms/LoopEmitter.cpp
61

Typo: Converts

62

tensor (singular)?

74

Typo: Converts

369

Using numbers and code is hard to read.
Just say

First, coord >= ...
Seconds, ...
etc.

401

insertion point

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

needs its own comment, or be moved some place else?

mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorCodegen.cpp
974

TODO, not "probably" ;-)

Peiming marked an inline comment as done.Feb 7 2023, 1:50 PM
Peiming added inline comments.
mlir/lib/Dialect/SparseTensor/IR/SparseTensorDialect.cpp
420

I remember that it crashed, as there is no fields for dimSliceAttr in specifier yet.

Peiming updated this revision to Diff 495662.Feb 7 2023, 3:40 PM
Peiming marked 8 inline comments as done.

address comments.

Peiming updated this revision to Diff 495665.Feb 7 2023, 3:48 PM
Peiming marked an inline comment as done.

address comments.

Peiming marked an inline comment as done.Feb 7 2023, 3:48 PM
aartbik accepted this revision.Feb 7 2023, 3:56 PM
This revision is now accepted and ready to land.Feb 7 2023, 3:56 PM
Peiming updated this revision to Diff 495890.Feb 8 2023, 10:57 AM

disable test case with memory leak (for now).

This revision was landed with ongoing or failed builds.Feb 8 2023, 10:58 AM
This revision was automatically updated to reflect the committed changes.