This is an archive of the discontinued LLVM Phabricator instance.

[mlir][sparse] merger extension to support sparsifying arith::CmpI/CmpF operation
ClosedPublic

Authored by Peiming on Jun 12 2023, 4:56 PM.

Diff Detail

Event Timeline

Peiming created this revision.Jun 12 2023, 4:56 PM
Herald added a project: Restricted Project. · View Herald Transcript
Peiming requested review of this revision.Jun 12 2023, 4:56 PM
aartbik added inline comments.Jun 12 2023, 7:06 PM
mlir/include/mlir/Dialect/SparseTensor/Utils/Merger.h
88

to generate (no s)

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

should we avoid reaching this altogether? I would hate for this to mess up with the semi-ring stuff....

1174–1189

I like the original v0/v1 a bit better, since lhs and rhs always reminds me of an assignment with lval and rval, and the vi corresponds to the ei in the children field

1184

can this be an else-if or can both branches be taken?!

mlir/lib/Dialect/SparseTensor/Utils/Merger.cpp
335

can we reach this "recursively" or is this always called from top?
If so, perhaps we need a new method, like

zeroMapSet(.)

and use that asserting on kCmpI/F and asserting on non-kCmpI/F here?

That would make a bit less special case iffing.

Peiming updated this revision to Diff 530971.Jun 13 2023, 10:21 AM
Peiming marked 5 inline comments as done.

address comments.

mlir/lib/Dialect/SparseTensor/Utils/Merger.cpp
335

Good suggestion!

Peiming updated this revision to Diff 531000.Jun 13 2023, 11:14 AM

add integration test.

Peiming updated this revision to Diff 531001.Jun 13 2023, 11:15 AM

fix errors.

aartbik added inline comments.Jun 14 2023, 11:28 AM
mlir/unittests/Dialect/SparseTensor/MergerTest.cpp
765

is this comment copy/paste?

802

same?

aartbik added inline comments.Jun 14 2023, 12:39 PM
mlir/lib/Dialect/SparseTensor/Transforms/Sparsification.cpp
1156–1161

remove empty line

1187

why not have a third

else {

v0 = genExp
 v1 = genExp

}
instead of this harder to read code? It would also avoid calling expExp twice if it returns Value

mlir/test/Integration/Dialect/SparseTensor/CPU/sparse_cmp.mlir
26

remove

90

remove

93

rephrase:

that constructs test matrices and calls....

123

period at end

Peiming updated this revision to Diff 531518.Jun 14 2023, 2:32 PM
Peiming marked 8 inline comments as done.

address comments.

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

You are right.

mlir/unittests/Dialect/SparseTensor/MergerTest.cpp
765

Fixed.

Peiming updated this revision to Diff 531519.Jun 14 2023, 2:33 PM

revert unintended change

Peiming updated this revision to Diff 531521.Jun 14 2023, 2:39 PM

fix format.

Peiming updated this revision to Diff 531522.Jun 14 2023, 2:40 PM

revert unintended changes.

Peiming updated this revision to Diff 531524.Jun 14 2023, 2:43 PM

minor fix.

aartbik accepted this revision.Jun 14 2023, 3:47 PM
This revision is now accepted and ready to land.Jun 14 2023, 3:47 PM
Peiming updated this revision to Diff 531814.Jun 15 2023, 10:15 AM

fix release build warning

This revision was landed with ongoing or failed builds.Jun 15 2023, 10:26 AM
This revision was automatically updated to reflect the committed changes.