This is an archive of the discontinued LLVM Phabricator instance.

[mlir][sparse] Refactoring sparsification using loop emitter.
AbandonedPublic

Authored by Peiming on Oct 11 2022, 3:42 PM.

Diff Detail

Event Timeline

Peiming created this revision.Oct 11 2022, 3:42 PM
Herald added a reviewer: aartbik. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
Peiming requested review of this revision.Oct 11 2022, 3:42 PM
Peiming updated this revision to Diff 466974.Oct 11 2022, 4:35 PM

fixed some check tests...

wrengr added inline comments.Oct 11 2022, 5:33 PM
mlir/lib/Dialect/SparseTensor/Transforms/CodegenUtils.cpp
173–179

This whole section is very confusing due to the variable names. Is dimType supposed to be a DimLevelType? If so, then why do you use is{Compressed,Singleton}Dim instead of is{Compressed,Singleton}DLT? and why is the field called dims rather than something like dimLevelTypes which would make it clear? If not, then what is dimType supposed to be?

mlir/lib/Dialect/SparseTensor/Transforms/CodegenUtils.h
297

I don't feel like this class really belongs in this file— since all the other things in CodegenUtils.{h,cpp} are small generic helpers, whereas this class is both rather large and pretty specific. Why not split it out into a separate LoopEmitter.{h,cpp} file?

324–335

Why have this as an inline definition rather than moving it to the cpp file like most of the other methods?

339–344

Ditto. While this one is pretty short, I'm don't imagine there's any benefit to having it be inline.

444

Rename ts to tensors and ds to dims.

447–450

Can any of these be marked const?

454–466

Move this documentation to just above the struct LoopSeqInfo { rather than having it inside the class definition

469

Constructors should be moved to the top of the class / fields should be at the bottom of the class

473–475

If this comment is for loopStack then it should be placed above the loopStack declaration. Whereas, if this comment is for loopSeqStack then it should have a blank line separating the comment from the loopStack declaration and shouldn't have any blank lines between the comment and the loopSeqStack declaration.

Peiming updated this revision to Diff 467524.Oct 13 2022, 10:19 AM

update check tests..

Peiming updated this revision to Diff 467525.Oct 13 2022, 10:22 AM

remove unsupported vector tests...

Peiming updated this revision to Diff 467536.Oct 13 2022, 10:48 AM
Peiming marked 7 inline comments as done.

address comments from Wren.

Peiming updated this revision to Diff 467537.Oct 13 2022, 10:50 AM
Peiming marked an inline comment as done.

mark varaible as const.

mlir/lib/Dialect/SparseTensor/Transforms/CodegenUtils.cpp
173–179

I think isCompressedDim looks good to me, I will leave the decision to Aart.

I will change the dims though! Thanks!

mlir/lib/Dialect/SparseTensor/Transforms/CodegenUtils.h
297

Sure, we can have some discussion on this!

324–335

Will do

447–450

You are right!

469

Deleted...

Peiming added inline comments.Oct 13 2022, 10:51 AM
mlir/lib/Dialect/SparseTensor/Transforms/CodegenUtils.h
444

I changed ts -> tids ( because there is a tensors field in the loop emitter that causes conflicts) hope it looks good to you ;-)

Peiming updated this revision to Diff 467544.Oct 13 2022, 11:12 AM

add comments.

Peiming updated this revision to Diff 467616.Oct 13 2022, 3:05 PM

cleanup..

aartbik added inline comments.Oct 17 2022, 9:41 PM
mlir/lib/Dialect/SparseTensor/Transforms/CodegenUtils.h
251

something went wrong with the rebase? this does not seem related?

269

you moved the class and changed it. perhaps this is a better place, but that makes the diff either harder to read.

can we first modify, and then later (once it is in), move the class, that makes it easier to read

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

This seems a functional change not really required for this revision, right?
perhaps you can leave that for later, since this is already rather hard to review?

408

a lot of these changes are because we removed vectorization

as discussed, that is okay, but this is something that can be done in a separate revision, right before this one, just so that the amount of changes per revision goes down

mlir/test/Integration/Dialect/SparseTensor/CPU/sparse_cast.mlir
7

Here and below, for all integration tests (not the CHECK tests) that are run without and with vectorization, simply remove the L7-L13 part. I will be easy to add that back later, so nothing is lost by removing it (in fact, at one point, we may want to run *all* integration tests with and without vectorization by some other means)

mlir/test/Dialect/SparseTensor/sparse_vector.mlir