This is an archive of the discontinued LLVM Phabricator instance.

[mlir][sparse]more integration test cases for sparse_tensor.BinaryOp
ClosedPublic

Authored by Peiming on Jun 22 2022, 2:45 PM.

Details

Summary

Adding more test cases for sparse_tensor.BinaryOp, including different cases when overlap/left/right region is implemented/empty/identity

Diff Detail

Event Timeline

Peiming created this revision.Jun 22 2022, 2:45 PM
Herald added a project: Restricted Project. · View Herald Transcript
Peiming requested review of this revision.Jun 22 2022, 2:45 PM
Peiming retitled this revision from more integration test cases for sparse_tensor.BinaryOp to [mlir][sparse]more integration test cases for sparse_tensor.BinaryOp.Jun 22 2022, 2:47 PM
Peiming added reviewers: bixia, wrengr, yinying-lisa-li.
Peiming updated this revision to Diff 439167.Jun 22 2022, 2:51 PM

format code

Peiming edited the summary of this revision. (Show Details)Jun 22 2022, 2:52 PM
Peiming updated this revision to Diff 439174.Jun 22 2022, 3:09 PM

add missing period...

Nice extra test!

To global nits:

(1) shall we either rename the file, or move the new code in its own file, since it is no longer just triangular binary

(2) always run asan on new integration tests to detect leaks (we are working on a more "comprehensive" bufferization with auto allocation story for sparse, literally ;-), but until then, we will need to insert proper alloc/deallocs ourselves

mlir/test/Integration/Dialect/SparseTensor/CPU/sparse_triangular_bin.mlir
0

you will need to release all the other sparse tensors too, but perhaps you can do that in print_result now, so we never skip one

1

It is actually funny this will achieve exactly the same as just addf as body ;-)

4

Here and below, any comment on its own line should look like a sentence, as in

// Define out-of-block constant bounds.

You can skip that rule for same line comment,as in

a = 1; // anything goes

117

Space after //, look like sentence

135

same

Peiming updated this revision to Diff 439437.Jun 23 2022, 9:37 AM

Fix memory leaking + format comments

Peiming updated this revision to Diff 439439.Jun 23 2022, 9:41 AM
Peiming marked 4 inline comments as done.

Use space instead of tab to indent code

Peiming updated this revision to Diff 439512.Jun 23 2022, 1:01 PM

merge file sparse_triangular_bin.mlir and sparse_binary.mlir

Solid work! I like it. A few last nits, but we are very close to submitting!

mlir/test/Integration/Dialect/SparseTensor/CPU/sparse_binary.mlir
39

either:
for sparse tensor.binary operations (plural ops)
or:
for a sparse_tensor.binary operation (indefinite article)

166–346

either remove the comment on this dump, or otherwise add a comment on all other dump functions too

265

It is nice that you threw in some extra testing of such cases.

Perhaps bonus points for a follow up CL where you accept constants in the generic block as well (like we do for linalg index). There is a very subtle problem there, but that makes it a nice follow up "starter" project.

mlir/test/Integration/Dialect/SparseTensor/CPU/sparse_triangular_bin.mlir
1

I won't ask this now, but for the future, typically when you move code from one place to another, and then change and/or add code, we break this up in two revisions, the NFC (no functional change) that moves the code, and then the one that changes, adds. That way it is a bit easier to review the deltas.

Peiming updated this revision to Diff 439895.Jun 24 2022, 2:31 PM
Peiming marked an inline comment as done.

Address Aart's comment

Peiming marked 2 inline comments as done.Jun 24 2022, 2:47 PM
mlir/test/Integration/Dialect/SparseTensor/CPU/sparse_triangular_bin.mlir
0

Done! I decided to put the release in entry function eventually as it is more clear.

1

Okay, got it!

aartbik accepted this revision.Jun 24 2022, 3:31 PM

Looking good! Ship it!

This revision is now accepted and ready to land.Jun 24 2022, 3:31 PM
Peiming added inline comments.Jun 27 2022, 10:43 AM
mlir/test/Integration/Dialect/SparseTensor/CPU/sparse_binary.mlir
265

Will take a look!