This is an archive of the discontinued LLVM Phabricator instance.

[mlir][sparse] Extend sparse_tensor.push_back to allow push_back a value n times.
ClosedPublic

Authored by bixia on Oct 24 2022, 5:40 PM.

Diff Detail

Event Timeline

bixia created this revision.Oct 24 2022, 5:40 PM
Herald added a project: Restricted Project. · View Herald Transcript
bixia requested review of this revision.Oct 24 2022, 5:40 PM
bixia updated this revision to Diff 470337.Oct 24 2022, 5:44 PM

Fix format.

Peiming accepted this revision.Oct 25 2022, 9:10 AM
This revision is now accepted and ready to land.Oct 25 2022, 9:10 AM
Peiming added inline comments.Oct 25 2022, 9:11 AM
mlir/include/mlir/Dialect/SparseTensor/IR/SparseTensorOps.td
275

Why can't we support n==1 though, it falls back to push_back, but should still be legitimate, right?

aartbik requested changes to this revision.Oct 25 2022, 11:34 AM
aartbik added inline comments.
mlir/include/mlir/Dialect/SparseTensor/IR/SparseTensorOps.td
275

Wait, is the n a compile-time constant?

That won't work? We need this as a regular SSA value.
The number of repeats will often depend on runtime computations?

This revision now requires changes to proceed.Oct 25 2022, 11:34 AM
aartbik added inline comments.Oct 25 2022, 12:24 PM
mlir/include/mlir/Dialect/SparseTensor/IR/SparseTensorOps.td
285

Perhaps also say something here that although this is semantically equivalent to calling push_back n times, it gives backend optimizations more chances to deal with memory reallocation and filling the memory with the same value.

bixia updated this revision to Diff 470613.Oct 25 2022, 1:59 PM

Make n a runtime value.

bixia added inline comments.Oct 25 2022, 2:00 PM
mlir/include/mlir/Dialect/SparseTensor/IR/SparseTensorOps.td
275

There must be some miscommunication, as in the discuss the decision was to use attribute. Change to use input value.

bixia updated this revision to Diff 470614.Oct 25 2022, 2:05 PM
bixia marked an inline comment as done.

Add description about the performance advantage.

aartbik accepted this revision.Oct 25 2022, 2:17 PM

Neat! Thanks!

mlir/include/mlir/Dialect/SparseTensor/IR/SparseTensorOps.td
274

layout?

275

No worries, thanks for changing. The indent was always to have runtime values, since we want to mimic e.g.

values.insert(values.end(), count, 0);

from the runtime library in actual code.

324

Interesting, when something is optional in the assembly, there is no builder without?

This revision is now accepted and ready to land.Oct 25 2022, 2:17 PM
bixia updated this revision to Diff 470825.Oct 26 2022, 8:44 AM
bixia marked an inline comment as done.

Fix format.