This is an archive of the discontinued LLVM Phabricator instance.

[mlir][sparse] add a cursor to sparse storage scheme
ClosedPublic

Authored by aartbik on Oct 26 2022, 3:08 PM.

Details

Summary

This prepare a subsequent revision that will generalize
the insertion code generation. Similar to the support lib,
insertions become much easier to perform with some "cursor"
bookkeeping. Note that we, in the long run, could perhaps
avoid storing the "cursor" permanently and use some
retricted-scope solution (alloca?) instead. However,
that puts harder restrictions on insertion-chain operations,
so for now we follow the more straightforward approach.

Diff Detail

Event Timeline

aartbik created this revision.Oct 26 2022, 3:08 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 26 2022, 3:08 PM
aartbik requested review of this revision.Oct 26 2022, 3:08 PM
Peiming added inline comments.Oct 26 2022, 3:30 PM
mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorCodegen.cpp
154

Do we need a memref for it? or a list of scalars is preferred? Will scalars expose more chances for optimization (maybe)?

aartbik added inline comments.Oct 26 2022, 3:54 PM
mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorCodegen.cpp
154

For starters, I think I really prefer the memref (also, it makes the storage scheme layout easier to understand, because otherwise we would introduce variable length header data). We will probably do a lot of j > a[0] comparisons, which are optimized quite well by mlir/llvm

aartbik updated this revision to Diff 471230.Oct 27 2022, 10:53 AM

rebased with main

Peiming added inline comments.Oct 27 2022, 10:54 AM
mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorCodegen.cpp
154

Okay, SG!

Peiming accepted this revision.Oct 27 2022, 11:02 AM

LGTM.

But I feel that the complexity of the sparse tensor layout keeps increasing, do you think it worth the effort to put all the index computation into its own class?

This revision is now accepted and ready to land.Oct 27 2022, 11:02 AM

But I feel that the complexity of the sparse tensor layout keeps increasing, do you think it worth the effort to put all the index computation into its own class?

We can consider that for sure! But I also like having the layout close to the codegen rewriting rules, so it is easy to scan back and forth in the file.
And FWIW, since we have now exactly the same fields as in the support library, I *think* we are done ;-) [famous last words]

More importantly, I am worried about passing too many parameters as memrefs, as we are starting to do (since lowering to LLVM IR gives elaborate code for that).
I would really like to pass around a single struct (tuple ;=) while still maintaining visibility in the individual memrefs....

This revision was landed with ongoing or failed builds.Oct 27 2022, 11:19 AM
This revision was automatically updated to reflect the committed changes.