This is an archive of the discontinued LLVM Phabricator instance.

[mlir][sparse] support affine expression on dense dimensions (except constant affine)
ClosedPublic

Authored by Peiming on Nov 16 2022, 5:48 PM.

Diff Detail

Event Timeline

Peiming created this revision.Nov 16 2022, 5:48 PM
Herald added a project: Restricted Project. · View Herald Transcript
Peiming requested review of this revision.Nov 16 2022, 5:48 PM
Peiming updated this revision to Diff 476231.Nov 17 2022, 1:56 PM

add test cases.

Peiming added inline comments.Nov 18 2022, 1:42 PM
mlir/lib/Dialect/SparseTensor/Transforms/Sparsification.cpp
258–266

It is actually prepared for filter loop idx (which is not yet introduced in this patch). But I keep it here to avoid merge conflict.

Peiming updated this revision to Diff 476600.Nov 18 2022, 2:04 PM

rename variables.

Peiming updated this revision to Diff 476617.Nov 18 2022, 3:22 PM

clang format.

aartbik added inline comments.Nov 18 2022, 5:07 PM
mlir/include/mlir/Dialect/SparseTensor/Utils/Merger.h
155

add comment on filter loops

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

move data to end (just the way google3 style works

make it private

1234

here and below, this is very subjective but I like

// Handle xxx
if (...)

return ....

or

if (...)

return ... // handle x

a bit better than

if (...)

// Handle x.
return

if there are no braces. But since there is no style for this, your choice in the end

Peiming updated this revision to Diff 476645.Nov 18 2022, 6:49 PM
Peiming marked 2 inline comments as done.

address comments.

Peiming added inline comments.Nov 18 2022, 6:52 PM
mlir/include/mlir/Dialect/SparseTensor/Utils/Merger.h
155

Filter loops are not yet introduced. Will add the comment in that patch to be more consistent.

aartbik added inline comments.Nov 21 2022, 2:10 PM
mlir/include/mlir/Dialect/SparseTensor/Utils/Merger.h
155

Yeah, not sure why i said that ;-)
Must have been staring at something else in the stack

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

please add a bit of doc to the class, especially since the method names are a bit obscure, e.g. setPickedIterType does not have direct meaning on who picks what ;-)

270

please doc what expansion means here

372

this is a huge block of code inside the iteration graph computation, can we move this into own function with intuitive name so that the original idea of

AffineExpr f = map.getResult(toOrigDim(enc, d - 1));
   AffineExpr t = map.getResult(toOrigDim(enc, d));
   addAffineOrderings(adjM, inDegree, f, t, 0);

is preserved? Otherwise it is very hard to separate intended logic from heuristic addition

606–607

this Note that comment is obsolete too now, right, or at least need some adjustmnet

Peiming added inline comments.Nov 21 2022, 2:13 PM
mlir/lib/Dialect/SparseTensor/Transforms/Sparsification.cpp
372

The reason why I started from 0 is actually due to filter loops... because the filter loop constraints are not imposed between (d - 1, d) but on (d) alone.

Maybe I should do the refactoring on the patch where filter loop is introduced?

aartbik accepted this revision.Nov 21 2022, 5:18 PM
aartbik added inline comments.
mlir/lib/Dialect/SparseTensor/Transforms/Sparsification.cpp
372

Ok, put a TODO here and then I accept the future promise ;-_

This revision is now accepted and ready to land.Nov 21 2022, 5:18 PM
Peiming updated this revision to Diff 477034.Nov 21 2022, 5:19 PM
Peiming marked 5 inline comments as done.

address comments.

aartbik accepted this revision.Nov 21 2022, 5:23 PM

Messages crossed, but except for the typo, LGTM stands

mlir/lib/Dialect/SparseTensor/Transforms/Sparsification.cpp
298–337

typo Constraints

Peiming updated this revision to Diff 477036.Nov 21 2022, 5:25 PM

fix typo.

This revision was landed with ongoing or failed builds.Nov 21 2022, 5:31 PM
This revision was automatically updated to reflect the committed changes.
Peiming added inline comments.Nov 21 2022, 5:36 PM
mlir/lib/Dialect/SparseTensor/Transforms/Sparsification.cpp
606–607

nice catch!