This is an archive of the discontinued LLVM Phabricator instance.

[mlir][sparse] add dim level type toString convenience method
ClosedPublic

Authored by aartbik on Dec 15 2022, 1:37 PM.

Diff Detail

Event Timeline

aartbik created this revision.Dec 15 2022, 1:37 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 15 2022, 1:37 PM
aartbik requested review of this revision.Dec 15 2022, 1:37 PM
aartbik updated this revision to Diff 483334.Dec 15 2022, 1:57 PM

moved to enums

aartbik updated this revision to Diff 483337.Dec 15 2022, 2:15 PM

semi colon ;-)

bixia accepted this revision.Dec 15 2022, 2:26 PM
bixia added inline comments.
mlir/include/mlir/Dialect/SparseTensor/IR/Enums.h
164

Why do we want to the double quote in the string?

This revision is now accepted and ready to land.Dec 15 2022, 2:26 PM
wrengr accepted this revision.Dec 15 2022, 2:31 PM
wrengr added inline comments.
mlir/include/mlir/Dialect/SparseTensor/IR/Enums.h
161

Rename to toMLIRString or similar? To make it clear that the string is à la MLIR surface syntax

161

Is this really worth inlining? or should the definition be moved to a cpp file instead?

mlir/lib/Dialect/SparseTensor/IR/SparseTensorDialect.cpp
163

You probably want to assert that the string is nonempty, just in case.

Also, you should keep the todo comment (unless it's been resolved by deciding that we shouldn't raise an error for undef).

aartbik marked 4 inline comments as done.Dec 15 2022, 2:42 PM
aartbik added inline comments.
mlir/include/mlir/Dialect/SparseTensor/IR/Enums.h
161

This way, we don't have dependences on a cpp file, which seemed nice given the nature of the Enums.h being pretty independent

161

that makes sense, used toMLIRString

164

I am neutral, but this way it matched the print/parser values. In the long run, when those are no longer strings we can change this one too

mlir/lib/Dialect/SparseTensor/IR/SparseTensorDialect.cpp
163

It feels like having a undef value is something that should be verified elsewhere, not while printing.
But I added to TODO back for now.

The nonempty string will never be reached with your unreachable follow up, so not really worth asserting now, right?

aartbik updated this revision to Diff 483354.Dec 15 2022, 2:50 PM
aartbik marked 4 inline comments as done.

addressed comments

aartbik updated this revision to Diff 483368.Dec 15 2022, 3:29 PM

rebased with main

This revision was landed with ongoing or failed builds.Dec 15 2022, 4:11 PM
This revision was automatically updated to reflect the committed changes.