This is an archive of the discontinued LLVM Phabricator instance.

[mlir][sparse] Add MLIR interface for sparse tensor file input and output.
ClosedPublic

Authored by bixia on Oct 7 2022, 12:09 PM.

Diff Detail

Event Timeline

bixia created this revision.Oct 7 2022, 12:09 PM
Herald added a project: Restricted Project. · View Herald Transcript
bixia requested review of this revision.Oct 7 2022, 12:09 PM
bixia updated this revision to Diff 466154.Oct 7 2022, 12:14 PM

Format the test.

wrengr added a comment.Oct 7 2022, 5:31 PM

Be sure to add these functions to the SparseTensorUtils.h header as well, and be sure to annotate them with the MLIR_CRUNNERUTILS_EXPORT macro, otherwise the generated code won't actually be allowed to call them.

mlir/lib/ExecutionEngine/SparseTensorUtils.cpp
546

I think these various wrappers for calling SparseTensorReader methods should be renamed to getSparseTensorReader{Rank,IsSymmetric,NNZ,...} —both to make it clear that they work on SparseTensorReader objects, and to avoid future/potential name clashes with similarly named method wrappers for other classes.

Or if you think those names are too long/ugly, then we should try to come up with some alternative naming scheme and apply it to all the functions exposed by this file. Maybe something like the naming schema st_{class}_{method}, where the st_ very succinctly indicates these are part of the sparse_tensor dialect, and splitting the class vs method names helps avoid some other irregularities in our current function names. Thus, we'd have st_reader_create, st_reader_getRank, etc for the stuff in this differential, but also st_storage_getValues, st_storage_getPointers, etc for stuff that's already there from before. (Or as a slight variation we could use the schema ST{Class}_{method}, i.e.: STReader_create, STStorage_getValues, etc.)

wrengr added a comment.EditedOct 7 2022, 5:31 PM

Oh, also be sure to actually rename the SparseTensorFile class to SparseTensorReader

Update: Ah nevermind. I see that's being done in D135477

bixia updated this revision to Diff 466425.Oct 9 2022, 10:42 PM
bixia marked an inline comment as done.

Add Reader/Writer to routine names for make it clear that the routines are for SparseTensorReader/Writer.
Add the routines to SparseTensorUtils.h and annotate the routines with MLIR_CRUNNERUTILS_EXPORT.

mlir/lib/ExecutionEngine/SparseTensorUtils.cpp
546

Thanks! Add Reader/Writer to the names.

bixia updated this revision to Diff 466822.Oct 11 2022, 8:04 AM

Rebase.

aartbik added inline comments.Oct 11 2022, 1:26 PM
mlir/lib/ExecutionEngine/SparseTensorUtils.cpp
618

minor nit, use index_type for the r var in the for loop as well, just to make sure we don't get issues on platforms with surprising index types

618

can we assert(rank != 0) before doing the rank-1 test?
or, iterate over all ranks and add spaces safe for the last one

mlir/test/Integration/Dialect/SparseTensor/CPU/sparse_file_io.mlir
166

Yeah, this is a neat feature to allow this kind of easy testing!

bixia updated this revision to Diff 466929.Oct 11 2022, 2:45 PM
bixia marked 2 inline comments as done.

Use index_type to look through rank. Add assert(rank!=0) before doing rank-1.

aartbik accepted this revision.Oct 11 2022, 2:52 PM
This revision is now accepted and ready to land.Oct 11 2022, 2:52 PM

Beware that you'll need to rebase this for D135613

mlir/include/mlir/ExecutionEngine/SparseTensorUtils.h
208–209

It's worth mentioning that this not only creates the object itself, but also opens the file and parses the header. (Both so that folks don't go looking for functions to do those steps, but also to make it clear that this can exit due to I/O or parsing errors.)

229

Might be worth mentioning that this also closes the file (just so folks don't go looking for a function to do that step).

239

Please switch this to the using syntax instead, to match the style used in the rest of include/mlir/ExecutionEngine/SparseTensor/*

244
mlir/lib/ExecutionEngine/SparseTensorUtils.cpp
582–588

I'd feel better if this code and its replica in openSparseTensorCOO were consolidated into a single method, in order to avoid the code duplication— especially since there's that 0-indexing vs 1-indexing conversion, so we really want to avoid the potential maintenance issues.

I can write up a quick CL for making that change in File.h if you don't want to do it in this CL

593

You shouldn't need this typedef here, since it's already given in the header file.

wrengr accepted this revision.Oct 11 2022, 3:14 PM

Forgot to hit the accept button :) Though do be sure to rebase after addressing the documentation nits

bixia updated this revision to Diff 466944.Oct 11 2022, 3:29 PM
bixia marked 5 inline comments as done.

Replace typedef with using. Modify function documentation. Remove an unnecessary typedef.

bixia updated this revision to Diff 466963.Oct 11 2022, 4:05 PM

Rebase.

wrengr added a subscriber: NathanielMcVicar.

This appears to have broken the C interface. I suppose the corresponding rename will be required there. https://lab.llvm.org/buildbot/#/builders/13/builds/26975/steps/6/logs/stdio

C:\buildbot\mlir-x64-windows-ninja\llvm-project\mlir\include\mlir/ExecutionEngine/SparseTensorRuntime.h(245): error C2220: the following warning is treated as an error
C:\buildbot\mlir-x64-windows-ninja\llvm-project\mlir\include\mlir/ExecutionEngine/SparseTensorRuntime.h(245): warning C4190: '_mlir_ciface_getSparseTensorReaderNextF16' has C-linkage specified, but returns UDT 'f16' which is incompatible with C
C:\buildbot\mlir-x64-windows-ninja\llvm-project\mlir\include\mlir/ExecutionEngine/Float16bits.h(35): note: see declaration of 'f16'

Sorry about this. Trying to fix it with https://reviews.llvm.org/D135811

mlir/include/mlir/ExecutionEngine/SparseTensorUtils.h
208–209

Add
This opens the file and read the header meta data based
of the sparse tensor format derived from the suffix of the file name.

229

Add
This also closes the file associated with
the reader.

mlir/lib/ExecutionEngine/SparseTensorUtils.cpp
582–588

Good point. I will do this in a follow up CL.

593

Removed, that was a mistake.