C Device API #31
Merged
C Device API #31
Conversation
This file contains 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
areusch
reviewed
Sep 7, 2021
rfcs/0031-devices-api.md
Outdated
| ``` | ||
|
|
||
| ## PrimFunc Resource Handle | ||
| A `tir::Var` is added to `PrimFunc` in `include/tvm/tir/function.h` which enables a `PrimFunc` to track and use the `resource_handle` parameter. This will be used by both unpacked and packed APIs to pass the resource down without packing into `TVMValue`, instead as a `void *`. |
There was a problem hiding this comment.
it kind of seems like we could get away without doing this by tracking which Target a function is going to run on and looking up the associated device API function in AOT/graph codegen. what do you think?
There was a problem hiding this comment.
We'd still need to pass this into the function call though to the operator though?
|
Thanks @Mousius . One thing to note about process. If we want to propose to change the TIR data structure(e.g. add resource handle field to PrimFunc), it would be great if we can have a separate RFC discuss process because the impact radius will be bigger and goes beyond micro TVM land. |
|
@areusch I've updated this and replied to your comments, could you please take another look - I believe implementing this is now blocked on the |
Mousius
added a commit
to Mousius/tvm
that referenced
this pull request
Sep 23, 2021
To address some of the concerns raised in apache#8280 and apache#8720 I've put together a series of functions to combine together names to re-use between these areas. These are meant to be a starting point to fix up the name generation to use the TVM C conventions and port the interface API header to C++. These functions will also be used for constructing the names in the C Device API (apache/tvm-rfcs#31).
Mousius
added a commit
to Mousius/tvm
that referenced
this pull request
Sep 23, 2021
To address some of the concerns raised in apache#8280 and apache#8720 I've put together a series of functions to combine together names to re-use between these areas. These are meant to be a starting point to fix up the name generation to use the TVM C conventions and port the interface API header to C++. These functions will also be used for constructing the names in the C Device API (apache/tvm-rfcs#31).
Mousius
added a commit
to Mousius/tvm
that referenced
this pull request
Sep 23, 2021
To address some of the concerns raised in apache#8280 and apache#8720 I've put together a series of functions to combine together names to re-use between these areas. These are meant to be a starting point to fix up the name generation to use the TVM C conventions and port the interface API header to C++. These functions will also be used for constructing the names in the C Device API (apache/tvm-rfcs#31).
Mousius
added a commit
to Mousius/tvm
that referenced
this pull request
Sep 23, 2021
To address some of the concerns raised in apache#8280 and apache#8720 I've put together a series of functions to combine together names to re-use between these areas. These are meant to be a starting point to fix up the name generation to use the TVM C conventions and port the interface API header to C++. These functions will also be used for constructing the names in the C Device API (apache/tvm-rfcs#31).
areusch
reviewed
Sep 23, 2021
rfcs/0031-devices-api.md
Outdated
| @@ -41,7 +41,7 @@ In this example, you can see the diversity of RTOS implementations for drivers a | |||
| [guide-level-explanation]: #guide-level-explanation | |||
|
|
|||
| ## User App | |||
| `tvm_device_<device>_t`s are implemented for each RTOS or platform required, these are included by the user who chooses as appropriate for their application. Notably, to avoid dynamic allocation, the user must provide the `tvm_device_<device>_t` struct and initialise it rather than it being created and setup for them in the API. This is augmented by named functions for each device, examples in the case of the "woofles" accelerator: | |||
| TVM presumes that the RTOS, platform, or user application defines a struct type `tvm_device_<device>_t`, these are included by the user who chooses an implementation as appropriate for their application. Notably, to avoid dynamic allocation, the user must provide the `tvm_device_<device>_t` struct and initialise it rather than it being created and setup for them in the API. TVM then expects an implementation of the named functions for each device, examples in the case of the "woofles" accelerator: | |||
There was a problem hiding this comment.
could you also explicitly highlight where <device> comes from from a user PoV?
There was a problem hiding this comment.
Is this not covered by the example just below this block?
There was a problem hiding this comment.
Nope, that was not the question, amended the text to clarify the origin.
| @@ -306,6 +293,8 @@ Another route to take is to treat RTOSes as entirely separate from TVM, requirin | |||
| # Unresolved questions | |||
| [unresolved-questions]: #unresolved-questions | |||
|
|
|||
| Is coupling `Target`s with this Device API to the C Runtime acceptable? From the authors point of view this seems an acceptable trade off given the devices won't function correctly without the flow control. | |||
There was a problem hiding this comment.
i think it's likely that TargetDevice will play a role here.
There was a problem hiding this comment.
Is there a TargetDevice RFC I can reference? Otherwise I'd assume TargetDevice is speculative rather than something we should include here?
There was a problem hiding this comment.
sorry not yet :/. it's sort of teased in the future work section of my Artifact pre-RFC. i'm more including this pointer here as an "early FYI" how i'd like to solve this (e.g. using the TargetDevice name as the mechanism to bind a sub-target to a Device API impl).
Mousius
added a commit
to Mousius/tvm
that referenced
this pull request
Sep 24, 2021
To address some of the concerns raised in apache#8280 and apache#8720 I've put together a series of functions to combine together names to re-use between these areas. These are meant to be a starting point to fix up the name generation to use the TVM C conventions and port the interface API header to C++. These functions will also be used for constructing the names in the C Device API (apache/tvm-rfcs#31).
|
@areusch this one is ready for a look again 😸 |
areusch
pushed a commit
to apache/tvm
that referenced
this pull request
Sep 30, 2021
* Introduce centralised name transformation functions To address some of the concerns raised in #8280 and #8720 I've put together a series of functions to combine together names to re-use between these areas. These are meant to be a starting point to fix up the name generation to use the TVM C conventions and port the interface API header to C++. These functions will also be used for constructing the names in the C Device API (apache/tvm-rfcs#31). * Improve error handling and error messages * Sanitize sanitise to sanitize This patch aims to sanitize uses of sanitise to the form of sanitize to be consistent with the overall codebases use of American English.
areusch
approved these changes
Oct 3, 2021
1 task
|
thanks @Mousius ! |
ylc
pushed a commit
to ylc/tvm
that referenced
this pull request
Jan 7, 2022
* Introduce centralised name transformation functions To address some of the concerns raised in apache#8280 and apache#8720 I've put together a series of functions to combine together names to re-use between these areas. These are meant to be a starting point to fix up the name generation to use the TVM C conventions and port the interface API header to C++. These functions will also be used for constructing the names in the C Device API (apache/tvm-rfcs#31). * Improve error handling and error messages * Sanitize sanitise to sanitize This patch aims to sanitize uses of sanitise to the form of sanitize to be consistent with the overall codebases use of American English.
ylc
pushed a commit
to ylc/tvm
that referenced
this pull request
Jan 13, 2022
* Introduce centralised name transformation functions To address some of the concerns raised in apache#8280 and apache#8720 I've put together a series of functions to combine together names to re-use between these areas. These are meant to be a starting point to fix up the name generation to use the TVM C conventions and port the interface API header to C++. These functions will also be used for constructing the names in the C Device API (apache/tvm-rfcs#31). * Improve error handling and error messages * Sanitize sanitise to sanitize This patch aims to sanitize uses of sanitise to the form of sanitize to be consistent with the overall codebases use of American English.
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.
No description provided.