-
Notifications
You must be signed in to change notification settings - Fork 3.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Hexagon VTCM and discontiguous allocation support #9525
Conversation
} | ||
|
||
void* HexagonBuffer::GetPointer() { | ||
void** HexagonBuffer::GetPointer() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Returning a void ** here disambiguates between the cases where there is a single allocation vs. multiple. We always return a pointer to a pointer. This may be controversial.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did we decide to revert this change so that tests pass and revisit once codegen supports pointer array indexing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I decided to keep HexagonBuffer
as-is returning a void**
which means that it (HexagonBuffer) supports discontiguous allocation. And, I noted the portions of the device API that force / assume contiguous allocation awaiting RFC 39 with "TODO" for myself.
6589f51
to
acc78e5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @adstraw, looking great! Partial review
e099787
to
e01dc93
Compare
98a413f
to
61db96e
Compare
c70256c
to
7122d2b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Requesting changes until we have verified with integration.
|
||
mod["add"](A_data, B_data, C_data) | ||
result = C_data.numpy() | ||
assert (result == np.array([6, 7])).all() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Safer to use all_close, and probably want to make another test where with larger tensors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's consider adding some more testing in a follow up.
98f152c
to
643e9ad
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks really great! Thanks @adstraw!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @adstraw, LGTM
Thank you for the great work @adstraw and for the review @csullivan ; the PR has been merged! |
* WIP Allocation abstraction for VTCM and DDR. * Add Hexagon VTCM and discontiguous allocation support * differentiate between dimensions and allocations * remove change to llvm codegen * add integration test_add_vtcm to demo vtcm alloc * remove cmake change * forcing contiguous allocation in device API, for now Co-authored-by: Chris Sullivan <csullivan@octoml.ai>
* WIP Allocation abstraction for VTCM and DDR. * Add Hexagon VTCM and discontiguous allocation support * differentiate between dimensions and allocations * remove change to llvm codegen * add integration test_add_vtcm to demo vtcm alloc * remove cmake change * forcing contiguous allocation in device API, for now Co-authored-by: Chris Sullivan <csullivan@octoml.ai>
* WIP Allocation abstraction for VTCM and DDR. * Add Hexagon VTCM and discontiguous allocation support * differentiate between dimensions and allocations * remove change to llvm codegen * add integration test_add_vtcm to demo vtcm alloc * remove cmake change * forcing contiguous allocation in device API, for now Co-authored-by: Chris Sullivan <csullivan@octoml.ai>
* WIP Allocation abstraction for VTCM and DDR. * Add Hexagon VTCM and discontiguous allocation support * differentiate between dimensions and allocations * remove change to llvm codegen * add integration test_add_vtcm to demo vtcm alloc * remove cmake change * forcing contiguous allocation in device API, for now Co-authored-by: Chris Sullivan <csullivan@octoml.ai>
* WIP Allocation abstraction for VTCM and DDR. * Add Hexagon VTCM and discontiguous allocation support * differentiate between dimensions and allocations * remove change to llvm codegen * add integration test_add_vtcm to demo vtcm alloc * remove cmake change * forcing contiguous allocation in device API, for now Co-authored-by: Chris Sullivan <csullivan@octoml.ai>
This PR uses
memcpy
. DMA support still pending. Code for VTCM allocation is present in this PR. VTCM allocation code is stubbed out when not building on Hexagon in favor or “mock” VTCM allocations usingmalloc
which allows for unit testing of theHexagonBuffer
class on CPU. The Hexagon Device API is modified to use theHexagonBuffer
class including implementation of theCopyDataFromTo
API.