This is an archive of the discontinued LLVM Phabricator instance.

[mlir][sparse] rename lex_insert into insert
ClosedPublic

Authored by aartbik on Sep 8 2022, 2:42 PM.

Details

Summary

This change goes not impact any semantics yet, but it
is in preparation for implementing the unordered and not-unique
properties. Changing lex_insert to insert is a first step.

Diff Detail

Event Timeline

aartbik created this revision.Sep 8 2022, 2:42 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 8 2022, 2:42 PM
aartbik requested review of this revision.Sep 8 2022, 2:42 PM
Peiming added inline comments.Sep 8 2022, 4:00 PM
mlir/include/mlir/Dialect/SparseTensor/IR/SparseTensorOps.td
208–218

Is it still the case if out-of-order insertion is allowed?

228

I think memref for array of indices make sense when we are using library.

Would an array<rank x index> (or even variadic inputs) be more straight forwards for codegen purpose?

Peiming added inline comments.Sep 8 2022, 4:03 PM
mlir/include/mlir/Dialect/SparseTensor/IR/SparseTensorOps.td
208–218

Is it still the case if out-of-order insertion is allowed?

Or even more aggressively, if user can guarantee two insert sessions happen in lex order, which prevents them from being able to append value to an already materialized tensor?

Peiming added inline comments.Sep 8 2022, 4:04 PM
mlir/include/mlir/Dialect/SparseTensor/IR/SparseTensorOps.td
208–218

Is it still the case if out-of-order insertion is allowed?

Or even more aggressively, if user can guarantee two insert sessions happen in lex order, which prevents them from being able to append value to an already materialized tensor?

Sorry, I mean what prevents them from being able to append value to an already materialized tensor...

Peiming added inline comments.Sep 8 2022, 4:06 PM
mlir/include/mlir/Dialect/SparseTensor/IR/SparseTensorOps.td
228

Say sparse_tensor.insert %t[%1, %2], %v : tensor<2x2xf64>, f64

aartbik marked 3 inline comments as done.Sep 8 2022, 4:59 PM
aartbik added inline comments.
mlir/include/mlir/Dialect/SparseTensor/IR/SparseTensorOps.td
208–218

Agreed we may relax to allow such in-place "fill-in" at some point, but for now I like to conservatively keep the "fresh output" behavior that is inherent to TACO's index expressions and relax the requirements when the cross that bridge

228

Yes, I was thinking of that too. We may benefit from the

store i, memref[0]
load j, memref[0] // rewrites i -> j

optimization in the codegen, but expressing this as SSA values will make this behavior much more clear

Peiming added inline comments.Sep 8 2022, 5:16 PM
mlir/include/mlir/Dialect/SparseTensor/IR/SparseTensorOps.td
228

So, will it be in the next patch? I believe it would be more than just changing test cases.

If so, the patch LGTM.

aartbik retitled this revision from [mlir][sparse] refined documented semantics of insertion op to [mlir][sparse] rename lex_insert into insert.Sep 8 2022, 5:18 PM
aartbik marked an inline comment as done.
aartbik added inline comments.
mlir/include/mlir/Dialect/SparseTensor/IR/SparseTensorOps.td
228

yeah that change will be much more involved; for now I really just wanted to remove the "lex" from "lex_insert". I changed the title to make that more clear.

Peiming accepted this revision.Sep 8 2022, 5:19 PM
This revision is now accepted and ready to land.Sep 8 2022, 5:19 PM
This revision was automatically updated to reflect the committed changes.