Support complex numbers for Matrix Market Exchange Formats. Add a test case.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
mlir/lib/ExecutionEngine/SparseTensorUtils.cpp | ||
---|---|---|
1264–1265 | NOTE: this now should distinguish between real, complex, and probably even integer! | |
1360–1370 | note that for the MM in particular, we should also verify that the data is marked real/complex/integer going forward, and verify that this is compatible with the actual data type <V> (assert, or otherwise fail when imcompatible) | |
1372 | I would add a comment on reading two doubles to make a complex here first, just so that it stays similar to L1317-1319. Otherwise, directly addressing the pattern issue reads a bit too terse. | |
1386–1387 | This is of course the right way forward, but did you measure perf impact of introducing more calls along the path that is executed for each nonzero? Perhaps try reading in one of the larger tensors before/after this change. Would marking methods as inline help here? | |
mlir/test/Integration/data/test_symmetric_complex.mtx | ||
2 | This must be "complex", not "real' following proper MM format |
Add data type check to MME header parsing.
Make the functions to read a COO value inline functions.
mlir/lib/ExecutionEngine/SparseTensorUtils.cpp | ||
---|---|---|
1360–1370 | Make the functions inline functions. |
mlir/lib/ExecutionEngine/SparseTensorUtils.cpp | ||
---|---|---|
1237 | This is of course a reasonable way to do it. But one may a bit surprised by having the fatal assert while reading the header. I feel that simply reading and storing the field in the SparseTensorFile class as an enum (with pattern, integer, real, complex, undef {to deal with FROSTT}) and then having getters will let the client decide what to do once the header is properly parsed seemed a bit more along the direction the refactoring in a class was taking. I don't feel super strongly about it, so this is more a WDYT? comment |
mlir/lib/ExecutionEngine/SparseTensorUtils.cpp | ||
---|---|---|
1263 | this is a very narrow subset of possibilities (i.e. must match exactly). I can see cases where we allow ints to be assigned to real and vv (as we did before), All such cases seem a lot easier to decide in the "client" (even if that is just us right now ;-) |
Use enum to store the value type to SparseTensorFile and check type compatible in openSparseTensorCOO.
I love it! Thanks for making the change. Two last nits, but good to go after that.
mlir/lib/ExecutionEngine/SparseTensorUtils.cpp | ||
---|---|---|
1299–1349 | Maybe add a comment, e.g. // The FROSTT format does not define the data type of the nonzero elements. | |
1367 | This does not seem to be exhaustive, i.e. complex vs isint seems an error too given that we have accepted real and int before, I would be perfectly okay that the only case we reject right now is complex file vs. non-complex data (which we can later refine into zero-imaginary part) |
Add a comment.
mlir/lib/ExecutionEngine/SparseTensorUtils.cpp | ||
---|---|---|
1367 | complex data vs integer tensor is an error. Maybe you misread tensorIsReal? It also includes integer. |
This is of course a reasonable way to do it. But one may a bit surprised by having the fatal assert while reading the header.
I feel that simply reading and storing the field in the SparseTensorFile class as an enum (with pattern, integer, real, complex, undef {to deal with FROSTT}) and then having getters will let the client decide what to do once the header is properly parsed seemed a bit more along the direction the refactoring in a class was taking.
I don't feel super strongly about it, so this is more a WDYT? comment