This is an archive of the discontinued LLVM Phabricator instance.

[mlir][sparse] Add AOS optimization.
ClosedPublic

Authored by bixia on Jan 2 2023, 9:49 PM.

Details

Summary

Use an array of structures to represent the indices for the tailing COO region
of a sparse tensor.

Diff Detail

Event Timeline

bixia created this revision.Jan 2 2023, 9:49 PM
Herald added a reviewer: aartbik. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
bixia requested review of this revision.Jan 2 2023, 9:49 PM
aartbik added inline comments.Jan 3 2023, 3:39 PM
mlir/lib/Dialect/SparseTensor/IR/SparseTensorDialect.cpp
266 ↗(On Diff #485918)

this is the only change in the file, and one I don't want ;-)
Can you please revert this file from the revision

mlir/lib/Dialect/SparseTensor/Transforms/CodegenUtils.cpp
10 ↗(On Diff #485918)

why this change? was lint complaining?

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

can't we pass an mutable as an immutable? if so, then it seems this is not a very pure distinction?

mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorStorageLayout.h
85

why the selective field/buffer renaming

also note that comments and method names are out of sync now at a lot of places

we use "field" here in the tuple sense, so I think I prefer keeping it that way, unless you have a very strong argument for the renaming?

bixia updated this revision to Diff 486113.Jan 3 2023, 4:05 PM
bixia marked 3 inline comments as done.

Removed an unnecessary include; reverted field/buffer renaming.

mlir/lib/Dialect/SparseTensor/IR/SparseTensorDialect.cpp
266 ↗(On Diff #485918)

This line and the routine after is added from the previous PR. There must be an issue with rebasing the source. Fixed.

mlir/lib/Dialect/SparseTensor/Transforms/CodegenUtils.cpp
10 ↗(On Diff #485918)

Removed.

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

We can't pass a mutable as an immutable or the other way.
Mutable keeps the linear buffers, immutable needs to explicitly create views of the linear buffers.

mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorStorageLayout.h
85

Revert the field/buffer change here and other places.

aartbik added inline comments.Jan 3 2023, 4:22 PM
mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorStorageLayout.h
75

here and below, I would even remove the "Disp"
does not add much information and only makes it longer

116

this class needs a comment now, describing what the template var means

307

either after the statement, or otherwise as a sentence Drop ... fields.

319

why is this so moved around? makes it hard to review?

can we do a prep revision with this, if needed ,first?

338

this changes the simplicity of the original setup quite a bit.

Can't we use template specialization to define the methods?
https://en.cppreference.com/w/cpp/language/template_specialization

bixia updated this revision to Diff 486333.Jan 4 2023, 10:06 AM
bixia marked 3 inline comments as done.

Split some change to https://reviews.llvm.org/D141002.
Address review comments.

bixia marked an inline comment as done.Jan 4 2023, 10:09 AM
bixia added inline comments.
mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorStorageLayout.h
75

Fix here and another place

116

Added description for the class. PTAL.

319
338

MutSparseTensorDescriptor now contains the getter/setter that are only meaningful to a mutable descriptor. PTAL.

bixia updated this revision to Diff 486379.Jan 4 2023, 1:21 PM
bixia marked an inline comment as done.

Rebase.

bixia updated this revision to Diff 486383.Jan 4 2023, 1:45 PM

Clang-format.

aartbik accepted this revision.Jan 4 2023, 3:13 PM
aartbik added inline comments.
mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorCodegen.cpp
304–305

why was there commented out code?

This revision is now accepted and ready to land.Jan 4 2023, 3:13 PM
bixia updated this revision to Diff 486413.Jan 4 2023, 3:36 PM
bixia marked an inline comment as done.

Remove commented out code.

This revision was automatically updated to reflect the committed changes.