This is an archive of the discontinued LLVM Phabricator instance.

[mlir][sparse] introduce sparse_tensor::StorageSpecifierType and related operations on it
ClosedPublic

Authored by Peiming on Dec 13 2022, 11:55 AM.

Diff Detail

Event Timeline

Peiming created this revision.Dec 13 2022, 11:55 AM
Herald added a project: Restricted Project. · View Herald Transcript
Peiming requested review of this revision.Dec 13 2022, 11:55 AM
Peiming retitled this revision from [mlir][sparse] introduce sparse_tensor::MetadateType and related operations on it to [mlir][sparse] introduce sparse_tensor::MetadataType and related operations on it.Dec 13 2022, 12:38 PM
Peiming updated this revision to Diff 482601.Dec 13 2022, 12:51 PM

update bazel file.

wrengr added inline comments.Dec 13 2022, 1:06 PM
mlir/include/mlir/Dialect/SparseTensor/IR/CMakeLists.txt
7–8

Since we already have the MLIRSparseTensorEnums library, you should rename these to something else to avoid confusion. Elsewhere in MLIR they often just append "enums" to the name of the file containing the definitions (e.g., SparseTensorAttrDefsEnums.{h,cpp}.inc; though I'd shorten that to SparseTensorAttrEnums.{h,cpp}.inc since that's more legible)

Peiming updated this revision to Diff 482614.Dec 13 2022, 1:19 PM

fix bazel

wrengr added inline comments.Dec 13 2022, 1:30 PM
mlir/include/mlir/Dialect/SparseTensor/IR/SparseTensorAttrDefs.td
177–180

Elsewhere we've been moving away from the "k" prefix on enum ctors. Do we really want the "k" here or no?

mlir/include/mlir/Dialect/SparseTensor/IR/SparseTensorOps.td
186–187

perhaps rephrase this to something more like "holds the sizes for dimensions, pointer arrays, index arrays, value arrays. This includes the buffer memory for storing those sizes, but not the buffer memory for the tensor itself." Though I'm not sure if the second sentence is really necessary (since imo it's implied by the preceding sentence).

wrengr added inline comments.Dec 13 2022, 1:47 PM
mlir/include/mlir/Dialect/SparseTensor/IR/SparseTensorOps.td
208

grammatically that should be "held by". Though I think it'd be better to say "Returns the requested field of the metadata."

212

"size of the"

212

This should be "level" rather than "dimension", and ditto for most (if not all) of the other places in this CL.

The one place I can think of where "dimension" is correct is for some of the uses of kDimSize. However, we will actually want to have both kDimSize and kLvlSize. See D139165 for more details about why we want both.

234

Grammatically that should be "Sets the requested field of". Though semantically it'd be better to rephrase the sentence as "Sets the field of the metadata to the given value."

239

"level".

239

"size of the"

Also it looks like you forgot to commit the contents of mlir/include/mlir/Dialect/SparseTensor/IR/SparseTensorTypes.h

Peiming updated this revision to Diff 482634.Dec 13 2022, 2:51 PM
Peiming marked 6 inline comments as done.

address comments.

Peiming retitled this revision from [mlir][sparse] introduce sparse_tensor::MetadataType and related operations on it to [mlir][sparse] introduce sparse_tensor::StorageSpecifierType and related operations on it.Dec 13 2022, 2:51 PM
Peiming added inline comments.
mlir/include/mlir/Dialect/SparseTensor/IR/SparseTensorAttrDefs.td
177–180

I personally have no preference

mlir/include/mlir/Dialect/SparseTensor/IR/SparseTensorOps.td
186–187

Agree, I did not include the second sentence.

Peiming updated this revision to Diff 482640.Dec 13 2022, 3:03 PM

remove leading k in enum class

Peiming marked 3 inline comments as done.Dec 13 2022, 3:07 PM
Peiming updated this revision to Diff 482661.Dec 13 2022, 4:54 PM

replace "sparse_tensor.storage_specifier" with "sparse_tensor.storage_specifier.init"

Peiming updated this revision to Diff 482662.Dec 13 2022, 4:55 PM

update doc.

aartbik added inline comments.Dec 13 2022, 10:19 PM
mlir/include/mlir/Dialect/SparseTensor/IR/SparseTensor.h
23

if you prefer, you can avoid all of this by simply repeating the define

#define GET_ATTRDEF_CLASSES
#include "mlir/Dialect/SparseTensor/IR/SparseTensorEnums.h.inc"

#define GET_ATTRDEF_CLASSES
#include "mlir/Dialect/SparseTensor/IR/SparseTensorAttrDefs.h.inc"

mlir/include/mlir/Dialect/SparseTensor/IR/SparseTensorAttrDefs.td
16

Introduce a new section here, and below for the new stuff
(and probably for the traits implementation)

just so we split the file a bit into parts

===----------------------------------------------------------------------===
Sparse Tensor Type Encoding Attribute
===----------------------------------------------------------------------===//

174

period at end

mlir/include/mlir/Dialect/SparseTensor/IR/SparseTensorTypes.td
33

is this still in the making?

74

newline?

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

can we move this to the top with the other #define/include parts above?
I realize we do this "mid file" trick at the end near L810, but that has its own reason

that way we have less includes spread out

Peiming updated this revision to Diff 482903.Dec 14 2022, 9:47 AM
Peiming marked 4 inline comments as done.

address comments from Aart.

mlir/include/mlir/Dialect/SparseTensor/IR/SparseTensor.h
23

I actually learned the trick for IREE through code search. But yeah, your way seems much more clear. Updated!

mlir/include/mlir/Dialect/SparseTensor/IR/SparseTensorTypes.td
33

Overlooked it... Now complete

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

Okay. make sense!

aartbik added inline comments.Dec 14 2022, 2:09 PM
mlir/include/mlir/Dialect/SparseTensor/IR/SparseTensorAttrDefs.td
178

still missin period after kind ;-)

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

Return an initial ....

Also, I think initial can use some explanation here

185

index arrays, and

(Oxford comma)

208

In all other ops, we either just say Example, or add explanation to the "header".
So

Example of querying the size ....

would be more consistent?

235

same suggestion, combine with Example header

mlir/include/mlir/Dialect/SparseTensor/IR/SparseTensorTypes.td
25

wait, this is not Sparse Tensor Types, but Storage Specifiers right? Different header?

Although we have a bit of a misnomer filename (SparseTensorTypes, should that be StorageSpecifierTypes?
(It is getting silly, I know ;-)

aartbik accepted this revision.Dec 14 2022, 2:12 PM

Approving pending addressing the comments.

This revision is now accepted and ready to land.Dec 14 2022, 2:12 PM
Peiming updated this revision to Diff 482991.Dec 14 2022, 2:19 PM
Peiming marked 8 inline comments as done.

address comments.

Peiming updated this revision to Diff 482992.Dec 14 2022, 2:26 PM

fix typo.

This revision was landed with ongoing or failed builds.Dec 14 2022, 2:27 PM
This revision was automatically updated to reflect the committed changes.