This is an archive of the discontinued LLVM Phabricator instance.

[mlir][sparse] Refactoring: remove dependence on tuple type when lowering sparse tensors.
ClosedPublic

Authored by Peiming on Sep 6 2022, 5:49 PM.

Diff Detail

Event Timeline

Peiming created this revision.Sep 6 2022, 5:49 PM
Herald added a project: Restricted Project. · View Herald Transcript
Peiming requested review of this revision.Sep 6 2022, 5:49 PM
Peiming updated this revision to Diff 458334.Sep 6 2022, 5:57 PM

Update comments

super neat! no more tuple collision, faster compile-time! nice!

mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorCodegen.cpp
79–80

this is no longer Optional, but just LogicalResult, since you have success() at L139

83

return failure()

368

please keep Location and also the place of this decl (after the comment on // Replace)

370–371

can we give this a better name then cast? I think the original tuple would still make a lot of sense here (as in a,b,c tuple, not tuple(a,b,c)).

411

move below // Construct

433

below // Replace

452–453

tuple?

479–480

tuple?

490–491

tuple?

mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorPasses.cpp
156

This is again, Everything!

186

commented out?

aartbik added inline comments.Sep 7 2022, 9:41 AM
mlir/test/Dialect/SparseTensor/codegen.mlir
251

make sure to indent the same, probably by moving all others to the left

272

same, indent by moving others left

Peiming updated this revision to Diff 458485.Sep 7 2022, 9:47 AM
Peiming marked 11 inline comments as done.

Address comments from Aart

mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorCodegen.cpp
79–80

This is a callback to the type converter. Return llvm::None tells the converter to try next converting rule.

370–371

I also abstracted the to_value/point/indices to share a SparseGetterOpConverter as the base class to reduce code repetition here.

Peiming marked 2 inline comments as done.Sep 7 2022, 9:56 AM
Peiming updated this revision to Diff 458491.Sep 7 2022, 9:58 AM

Indent checks

Peiming updated this revision to Diff 458493.Sep 7 2022, 10:05 AM

add missing periods...

Peiming updated this revision to Diff 458495.Sep 7 2022, 10:12 AM

fix comment

aartbik added inline comments.Sep 7 2022, 10:24 AM
mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorCodegen.cpp
57

tenors -> tensors

79–80

you are right!

495

Note that in the original rule, I returned failure(), i.e. the rewriting does not occur (which is the idiom used elsehwere too for stuff the sparse compiler cannot handle). We typically generate these ops ourselves, so they are always constant index. But just in case they are not, it seems safer to not touch them. An assert only happens in debug mode, in prod, we will crash on L491

While writing these rules, I was thinking that we should really put in the restriction in the op itself. The variable case *can* be handled, but requires setting up some indexing array to get the field at runtime. Does not seem worth it.

511

Is it possible to let Base::getFieldForOp() return unsigned field.
If so, you can push

assert(i < fields.size());
return fields[i];

into the base class, and make all derived classes even shorter?

Peiming updated this revision to Diff 458511.Sep 7 2022, 10:43 AM
Peiming marked 4 inline comments as done.

Address comments from Aart

mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorCodegen.cpp
57

Nice catch!

511

Good suggestion!

aartbik accepted this revision.Sep 7 2022, 10:50 AM
This revision is now accepted and ready to land.Sep 7 2022, 10:50 AM
This revision was landed with ongoing or failed builds.Sep 7 2022, 10:53 AM
This revision was automatically updated to reflect the committed changes.
mlir/test/Dialect/SparseTensor/roundtrip.mlir