fix: parse integer MTX files and fix tensor buffer ownership#580
Open
urays wants to merge 1 commit into
Open
Conversation
Accept MatrixMarket integer fields alongside real fields, transfer unpacked tensor index/value buffers to Array ownership, and avoid leaking copied fill values.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR fixes two sparse tensor correctness issues:
integerfields to be parsed in addition toreal.Problem
TACO previously rejected valid MatrixMarket files whose field type was
integer, even though those files can behandled by the existing MatrixMarket read path.
TACO also leaked heap memory after packing or reading sparse tensors. The affected allocations came from generated
pack/assemble kernels and from
taco_tensor_tfill-value handling.There were two ownership issues:
init_taco_tensor_tallocated a separatefill_value, butdeinit_taco_tensor_tnever released it. In practice,TensorStorage::operator taco_tensor_t*()may also replacetensorData->fill_valuewithcontent->fillValue.getValPtr(), so treatingfill_valueas owned storage is unsafe.unpackTensorDataandKernel::unpackResultswrapped generated sparsepos,idx, andvalsbuffers withArray::UserOwns. These buffers are allocated by generated TACO code usingmalloc/realloc; after transfer intoTensorStorage, there is no other owner, so they must be released by the receivingArray.Changes
integerfields alongsiderealfields.Array::Freeinstead ofArray::UserOwns.Kernel::unpackResultsto use the same ownership behavior for generated sparse result buffers.init_taco_tensor_tsofill_valuealiases the suppliedfill_ptrrather than allocating a separate copy.taco_mode_dia1.Validation
Valgrind was run on a MatrixMarket CSR read + SpMM case that previously reported definitely-lost allocations from
generated pack code and
taco_tensor_tinitialization.After this change:
Notes
The ownership model is now explicit: generated sparse buffers are transferred into TACO storage and released through
Array::Free, while taco_tensor_t::fill_value remains a non-owning reference to storage managed by the TensorStorage/
Literal path.