Fix ToDlPack memory space classification for offset views - #1238
Merged
Conversation
The comment described that it performs a next address approximation on the input pointer. This is not what the implementation does. Instead, it performs an exact lookup with the key being the input pointer. Thus, the base pointer of the tensor needs to be passed in.
- make GetStorage const - use tensor_t::GetStorage::data instead of tensor_t::Data as the input for GetPointerKind in order to make the memory space lookup possible with Slice/Permutation views
- Add DLPackTestsAll.ExportOffsetSlice and ExportPermutedOffsetSlice to cover views whose internal data pointer is offset from the base pointer - GetPointerKind is an exact address lookup, so an offset pointer is never found, so ToDlPack must use the base pointer to classify the memory space - Refactor the per memory space loop into a shared helper
Rewords the note about base-pointer-only lookups and removes the stale mention of a previously-supported approximate lookup.
tensor_impl_t and dynamic_tensor_t don't expose GetStorage(), so the direct op.GetStorage().data() call in PrintData didn't compile for those types. GetStorageBasePointer() falls back to op.Data() for them via a requires-clause, while still passing the true base pointer for types that do carry storage.
The existing GetStorage() returns storage_ by value, bumping the shared buffer's reference count on every call. Callers that only need to read through storage (e.g. GetStorageBasePointer() in print.h) can now bind to a const tensor and get a reference instead. The non-const overload keeps returning a copy for callers that need to hand ownership elsewhere (make_tensor(), sparse tensor constructors).
Covers the two shapes PrintData() must classify: a slice whose Data() is offset into its allocation, and tensor_impl_t, which carries no storage and must fall back to Data(). The tensor_impl_t case guards against the build regression GetStorageBasePointer() fixes.
RealView()/ImagView() build their own non-owning storage at the offset/reinterpreted pointer instead of sharing the source tensor's storage_, so GetStorage().data() on the result never resolves to the allocation's true base. ToDlPackImpl's allocator-map lookup always misses for them and falls back to cuPointerGetAttributes, but that fallback only checked CU_MEMORYTYPE_DEVICE and dumped everything else (including genuinely pinned host memory) into kDLCPU. Verified empirically (probe against the driver API on this Jetson target) that CU_POINTER_ATTRIBUTE_IS_MANAGED distinguishes managed memory from pinned host memory, both of which report CU_MEMORYTYPE_HOST via CU_POINTER_ATTRIBUTE_MEMORY_TYPE alone. The fallback now checks both attributes and classifies exactly like the exact-match path: managed/device -> kDLCUDA, pinned host -> kDLCUDAHost, else -> kDLCPU. Also documents on RealView()/ImagView() why they always hit this fallback path rather than the allocator-map lookup.
Covers the fallback path ToDlPackImpl() takes for RealView()/ImagView(): their non-owning storage never resolves to the allocation's true base, so GetPointerKind()'s exact-match lookup always misses and cuPointerGetAttributes must classify the memory instead. Slicing first guarantees both views start at a non-zero offset, exercising the fix across all five exportable memory spaces.
lennartvoelz
marked this pull request as ready for review
August 26, 2026 11:28
Contributor
Greptile SummaryThis PR corrects memory-space classification for offset tensor views while preserving the exported view pointer and metadata.
Confidence Score: 5/5The PR appears safe to merge, with no concrete changed-code defect identified. The implementation separates the exported offset data pointer from the allocation pointer used for memory classification, preserves DLPack shape, stride, and lifetime behavior, and retains fallback handling for views whose storage pointer is not an allocator-recorded base. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart LR
V[Tensor offset view] --> D[Export Data pointer]
V --> S[Underlying storage base pointer]
S --> M[Allocator memory-space lookup]
M --> T[DLPack device metadata]
D --> C[DLPack consumer]
T --> C
Reviews (1): Last reviewed commit: "Add DLPack export test for RealView()/Im..." | Re-trigger Greptile |
Collaborator
|
/build |
|
Coverage is 93.399% — lennartvoelz:bug/fix_ToDlPack into NVIDIA:main. No base build found for |
cliffburdick
approved these changes
Aug 26, 2026
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
3 participants
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.
Fixes Issue #1235
Fix
Classify from the storage base pointer instead of from the possibly offset data pointer.
The exported tensor is unchaned, so dl_tensor.data is still the views offset pointer. Only the memory probe is moved to the base pointer of the underlying storage.
Changes
include/matx/core/tensor.h: ToDlPackImpl() classifies from GetStorage().data(); GetStorage() made const so a const tensor can be exportedinclude/matx/core/print.h: same defect in PrintData(), same fixinclude/matx/core/allocator.h: GetPointerKind()'s doc comment described an approximate "next lowest address" lookup that the implementation does not do. Rewritten to state the actual contract: exact match, base pointer required, MATX_INVALID_MEMORY on a missTesting
DLPackTestsAll.ExportOffsetSlice and .ExportPermutedOffsetSlice, over MatXAllTypesCUDAExec and all five memory spaces, covering both the legacy and versioned export paths; each asserts the exported device type, dtype, shape and strides, that data is the view's offset pointer with byte_offset == 0, and that the storage refcount returns to its pre-export value after deleter().
Verification
test_00_tensor_DLPackTests: 84 passed.
Confirmed the tests actually pin the fix: reverting ToDlPackImpl() to this->Data() fails 26 cases across ExportOffsetSlice and ExportPermutedOffsetSlice.