You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tvm.apache.org by GitBox <gi...@apache.org> on 2021/05/11 21:03:57 UTC
[GitHub] [tvm] manupa-arm opened a new pull request #8019: Adding workspace byte alignment
manupa-arm opened a new pull request #8019:
URL: https://github.com/apache/tvm/pull/8019
* This commit adds byte alignment support for workspaces
* Updating AoT tests to use calculate workspaces
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] areusch commented on a change in pull request #8019: [uTVM][AOT] Adding workspace byte alignment
Posted by GitBox <gi...@apache.org>.
areusch commented on a change in pull request #8019:
URL: https://github.com/apache/tvm/pull/8019#discussion_r631956556
##########
File path: include/tvm/runtime/crt/stack_allocator.h
##########
@@ -45,14 +45,61 @@ typedef struct {
size_t workspace_size; // Total number of bytes in the workspace
} tvm_workspace_t;
+/*!
+ * \brief Initialize the stack-based memory manager
+ *
+ * \param tvm_runtime_workspace The tvm_workspace_t struct containing state
+ * \param g_aot_memory The memory buffer used to allocate within
+ * \param workspace_size The total size of the workspace buffer workspace
+ */
tvm_crt_error_t StackMemoryManager_Init(tvm_workspace_t* tvm_runtime_workspace,
uint8_t* g_aot_memory, size_t workspace_size);
+/*!
+ * \brief The intended user-facing function to allocate within the buffer. It wraps
+ * StackMemoryManager_Allocate_Body enable and disable the FIFO check that is useful for debugging
+ * the AoT codegen.
+ *
+ * \param tvm_runtime_workspace The tvm_workspace_t struct containing state
+ * \param nbytes The number of bytes required for the allocation
+ * \param current_alloc The pointer-to-pointer to be populated with the allocated address
+ */
tvm_crt_error_t StackMemoryManager_Allocate(tvm_workspace_t* tvm_runtime_workspace, int32_t nbytes,
- void**);
+ void** current_alloc);
+
+/*!
+ * \brief The internal function that accepts allocate inputs and an extra byte to say to enable the
+ * FIFO check that is useful in debugging for debugging the AoT codegen.
+ *
+ * \param tvm_runtime_workspace The tvm_workspace_t struct containing state
+ * \param nbytes The number of bytes required for the allocation
+ * \param current_alloc The pointer-to-pointer to be populated with the allocated address
+ * \param do_fifo_check THis indicates to perform a check FIFO pattern Allocs/Frees
Review comment:
just saying to be explicit about which boolean value means what. Usually I write stuff like this as "true when this function should explicitly check for a FILO pattern of Alloc/Frees"
(also I realized yesterday that I've been using FIFO but it's wrong word here :) )
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] manupa-arm commented on a change in pull request #8019: [uTVM][AOT] Adding workspace byte alignment
Posted by GitBox <gi...@apache.org>.
manupa-arm commented on a change in pull request #8019:
URL: https://github.com/apache/tvm/pull/8019#discussion_r631943300
##########
File path: src/tir/analysis/calculate_workspace.cc
##########
@@ -46,6 +48,10 @@ size_t WorkspaceCalculator::operator()(const PrimFunc& func) {
return this->max_size;
}
+size_t WorkspaceCalculator::GetByteAlignedSize(size_t non_aligned_size) {
+ return ((non_aligned_size + byte_alignment - 1) / byte_alignment) * byte_alignment;
Review comment:
Yes, that is right -- in that work we'll just directly encode the offsets within the buffer that can respect the alignment. Until that we can have a global (sub-optimal) alignment accross all tensors accessed. Does that make sense?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] manupa-arm commented on a change in pull request #8019: [uTVM][AOT] Adding workspace byte alignment
Posted by GitBox <gi...@apache.org>.
manupa-arm commented on a change in pull request #8019:
URL: https://github.com/apache/tvm/pull/8019#discussion_r631539748
##########
File path: src/tir/analysis/calculate_workspace.cc
##########
@@ -46,6 +48,10 @@ size_t WorkspaceCalculator::operator()(const PrimFunc& func) {
return this->max_size;
}
+size_t WorkspaceCalculator::GetByteAlignedSize(size_t non_aligned_size) {
+ return ((non_aligned_size + byte_alignment - 1) / byte_alignment) * byte_alignment;
Review comment:
That is expected to be aligned by user in the application. E.g.,
` __attribute__((section("SRAM1"), aligned(16))) static uint8_t workspace_buffer [WORKSPACE_SIZE];`
` extern tvm_model_t network;`
` tvm_workspace_t app_workspace;`
int main(...) {
...
StackMemoryManager_Init(&app_workspace, workspace_buffer, WORKSPACE_SIZE);
tvm_runtime_run(&network, inputs, outputs);
}
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] areusch commented on pull request #8019: [uTVM][AOT] Adding workspace byte alignment
Posted by GitBox <gi...@apache.org>.
areusch commented on pull request #8019:
URL: https://github.com/apache/tvm/pull/8019#issuecomment-840180301
@manupa-arm thanks for putting this up! did an initial pass and left some comments
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] manupa-arm commented on a change in pull request #8019: [uTVM][AOT] Adding workspace byte alignment
Posted by GitBox <gi...@apache.org>.
manupa-arm commented on a change in pull request #8019:
URL: https://github.com/apache/tvm/pull/8019#discussion_r632428367
##########
File path: src/tir/analysis/calculate_workspace.cc
##########
@@ -33,10 +33,12 @@ class WorkspaceCalculator : public StmtExprVisitor {
public:
WorkspaceCalculator() = default;
size_t operator()(const PrimFunc& func);
+ size_t byte_alignment = 1;
Review comment:
Done!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] areusch commented on a change in pull request #8019: [uTVM][AOT] Adding workspace byte alignment
Posted by GitBox <gi...@apache.org>.
areusch commented on a change in pull request #8019:
URL: https://github.com/apache/tvm/pull/8019#discussion_r631478754
##########
File path: src/tir/analysis/calculate_workspace.cc
##########
@@ -46,6 +48,10 @@ size_t WorkspaceCalculator::operator()(const PrimFunc& func) {
return this->max_size;
}
+size_t WorkspaceCalculator::GetByteAlignedSize(size_t non_aligned_size) {
+ return ((non_aligned_size + byte_alignment - 1) / byte_alignment) * byte_alignment;
Review comment:
if we calculate alignment here, what happens if the workspace base is not itself aligned?
##########
File path: include/tvm/runtime/crt/stack_allocator.h
##########
@@ -45,14 +45,61 @@ typedef struct {
size_t workspace_size; // Total number of bytes in the workspace
} tvm_workspace_t;
+/*!
+ * \brief Initialize the stack-based memory manager
+ *
+ * \param tvm_runtime_workspace The tvm_workspace_t struct containing state
+ * \param g_aot_memory The memory buffer used to allocate within
+ * \param workspace_size The total size of the workspace buffer workspace
+ */
tvm_crt_error_t StackMemoryManager_Init(tvm_workspace_t* tvm_runtime_workspace,
uint8_t* g_aot_memory, size_t workspace_size);
+/*!
+ * \brief The intended user-facing function to allocate within the buffer. It wraps
+ * StackMemoryManager_Allocate_Body enable and disable the FIFO check that is useful for debugging
+ * the AoT codegen.
+ *
+ * \param tvm_runtime_workspace The tvm_workspace_t struct containing state
+ * \param nbytes The number of bytes required for the allocation
+ * \param current_alloc The pointer-to-pointer to be populated with the allocated address
+ */
tvm_crt_error_t StackMemoryManager_Allocate(tvm_workspace_t* tvm_runtime_workspace, int32_t nbytes,
- void**);
+ void** current_alloc);
+
+/*!
+ * \brief The internal function that accepts allocate inputs and an extra byte to say to enable the
+ * FIFO check that is useful in debugging for debugging the AoT codegen.
+ *
+ * \param tvm_runtime_workspace The tvm_workspace_t struct containing state
+ * \param nbytes The number of bytes required for the allocation
+ * \param current_alloc The pointer-to-pointer to be populated with the allocated address
+ * \param do_fifo_check THis indicates to perform a check FIFO pattern Allocs/Frees
Review comment:
specify the polarity
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] manupa-arm commented on a change in pull request #8019: [uTVM][AOT] Adding workspace byte alignment
Posted by GitBox <gi...@apache.org>.
manupa-arm commented on a change in pull request #8019:
URL: https://github.com/apache/tvm/pull/8019#discussion_r631539748
##########
File path: src/tir/analysis/calculate_workspace.cc
##########
@@ -46,6 +48,10 @@ size_t WorkspaceCalculator::operator()(const PrimFunc& func) {
return this->max_size;
}
+size_t WorkspaceCalculator::GetByteAlignedSize(size_t non_aligned_size) {
+ return ((non_aligned_size + byte_alignment - 1) / byte_alignment) * byte_alignment;
Review comment:
That is expected to be aligned by user in the application. E.g.,
` __attribute__((section("SRAM1"), aligned(16))) static uint8_t workspace_buffer [WORKSPACE_SIZE];
extern tvm_model_t network;
tvm_workspace_t app_workspace;
int main(...) {
...
StackMemoryManager_Init(&app_workspace, workspace_buffer, WORKSPACE_SIZE);
tvm_runtime_run(&network, inputs, outputs);
}`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] manupa-arm commented on a change in pull request #8019: [uTVM][AOT] Adding workspace byte alignment
Posted by GitBox <gi...@apache.org>.
manupa-arm commented on a change in pull request #8019:
URL: https://github.com/apache/tvm/pull/8019#discussion_r631943667
##########
File path: include/tvm/runtime/crt/stack_allocator.h
##########
@@ -45,14 +45,61 @@ typedef struct {
size_t workspace_size; // Total number of bytes in the workspace
} tvm_workspace_t;
+/*!
+ * \brief Initialize the stack-based memory manager
+ *
+ * \param tvm_runtime_workspace The tvm_workspace_t struct containing state
+ * \param g_aot_memory The memory buffer used to allocate within
+ * \param workspace_size The total size of the workspace buffer workspace
+ */
tvm_crt_error_t StackMemoryManager_Init(tvm_workspace_t* tvm_runtime_workspace,
uint8_t* g_aot_memory, size_t workspace_size);
+/*!
+ * \brief The intended user-facing function to allocate within the buffer. It wraps
+ * StackMemoryManager_Allocate_Body enable and disable the FIFO check that is useful for debugging
+ * the AoT codegen.
+ *
+ * \param tvm_runtime_workspace The tvm_workspace_t struct containing state
+ * \param nbytes The number of bytes required for the allocation
+ * \param current_alloc The pointer-to-pointer to be populated with the allocated address
+ */
tvm_crt_error_t StackMemoryManager_Allocate(tvm_workspace_t* tvm_runtime_workspace, int32_t nbytes,
- void**);
+ void** current_alloc);
+
+/*!
+ * \brief The internal function that accepts allocate inputs and an extra byte to say to enable the
+ * FIFO check that is useful in debugging for debugging the AoT codegen.
+ *
+ * \param tvm_runtime_workspace The tvm_workspace_t struct containing state
+ * \param nbytes The number of bytes required for the allocation
+ * \param current_alloc The pointer-to-pointer to be populated with the allocated address
+ * \param do_fifo_check THis indicates to perform a check FIFO pattern Allocs/Frees
Review comment:
Sorry I dont follow, could you clarify a bit more?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] manupa-arm commented on a change in pull request #8019: [uTVM][AOT] Adding workspace byte alignment
Posted by GitBox <gi...@apache.org>.
manupa-arm commented on a change in pull request #8019:
URL: https://github.com/apache/tvm/pull/8019#discussion_r631539748
##########
File path: src/tir/analysis/calculate_workspace.cc
##########
@@ -46,6 +48,10 @@ size_t WorkspaceCalculator::operator()(const PrimFunc& func) {
return this->max_size;
}
+size_t WorkspaceCalculator::GetByteAlignedSize(size_t non_aligned_size) {
+ return ((non_aligned_size + byte_alignment - 1) / byte_alignment) * byte_alignment;
Review comment:
That is expected to be aligned by user in the application. E.g.,
```
__attribute__((section("SRAM1"), aligned(16))) static uint8_t workspace_buffer [WORKSPACE_SIZE];
extern tvm_model_t network;`
tvm_workspace_t app_workspace;
int main(...) {
...
StackMemoryManager_Init(&app_workspace, workspace_buffer, WORKSPACE_SIZE);
tvm_runtime_run(&network, inputs, outputs);
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] areusch merged pull request #8019: [uTVM][AOT] Adding workspace byte alignment
Posted by GitBox <gi...@apache.org>.
areusch merged pull request #8019:
URL: https://github.com/apache/tvm/pull/8019
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] manupa-arm commented on pull request #8019: [uTVM][AOT] Adding workspace byte alignment
Posted by GitBox <gi...@apache.org>.
manupa-arm commented on pull request #8019:
URL: https://github.com/apache/tvm/pull/8019#issuecomment-841154689
I think I've addressed all of you comments
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] manupa-arm commented on a change in pull request #8019: [uTVM][AOT] Adding workspace byte alignment
Posted by GitBox <gi...@apache.org>.
manupa-arm commented on a change in pull request #8019:
URL: https://github.com/apache/tvm/pull/8019#discussion_r632050646
##########
File path: src/tir/analysis/calculate_workspace.cc
##########
@@ -33,10 +33,12 @@ class WorkspaceCalculator : public StmtExprVisitor {
public:
WorkspaceCalculator() = default;
size_t operator()(const PrimFunc& func);
+ size_t byte_alignment = 1;
Review comment:
How about kWorkspaceAllocAlignment in include/tvm/runtime/device_api.h ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] manupa-arm commented on a change in pull request #8019: [uTVM][AOT] Adding workspace byte alignment
Posted by GitBox <gi...@apache.org>.
manupa-arm commented on a change in pull request #8019:
URL: https://github.com/apache/tvm/pull/8019#discussion_r631995595
##########
File path: src/tir/analysis/calculate_workspace.cc
##########
@@ -46,6 +48,10 @@ size_t WorkspaceCalculator::operator()(const PrimFunc& func) {
return this->max_size;
}
+size_t WorkspaceCalculator::GetByteAlignedSize(size_t non_aligned_size) {
+ return ((non_aligned_size + byte_alignment - 1) / byte_alignment) * byte_alignment;
Review comment:
Well; I've made sure it defaults to 1 wherever its accessed, so should not affect anything else if its not specified.
This enables us to see where we are in-terms of memory usage (by actually running models using the calculated value) -- that motivates the unified memory planning work even more.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] areusch commented on a change in pull request #8019: [uTVM][AOT] Adding workspace byte alignment
Posted by GitBox <gi...@apache.org>.
areusch commented on a change in pull request #8019:
URL: https://github.com/apache/tvm/pull/8019#discussion_r631978186
##########
File path: src/tir/analysis/calculate_workspace.cc
##########
@@ -46,6 +48,10 @@ size_t WorkspaceCalculator::operator()(const PrimFunc& func) {
return this->max_size;
}
+size_t WorkspaceCalculator::GetByteAlignedSize(size_t non_aligned_size) {
+ return ((non_aligned_size + byte_alignment - 1) / byte_alignment) * byte_alignment;
Review comment:
okay, i think that makes sense to me. i do wonder if it makes sense to upstream this given we're just going to change it later, but given the target parameter does not have a default, it's probably fine to do this as an interim solution until we address the broader memory planning challenges.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] areusch commented on a change in pull request #8019: [uTVM][AOT] Adding workspace byte alignment
Posted by GitBox <gi...@apache.org>.
areusch commented on a change in pull request #8019:
URL: https://github.com/apache/tvm/pull/8019#discussion_r631904664
##########
File path: src/tir/analysis/calculate_workspace.cc
##########
@@ -46,6 +48,10 @@ size_t WorkspaceCalculator::operator()(const PrimFunc& func) {
return this->max_size;
}
+size_t WorkspaceCalculator::GetByteAlignedSize(size_t non_aligned_size) {
+ return ((non_aligned_size + byte_alignment - 1) / byte_alignment) * byte_alignment;
Review comment:
okay sure, but also since this calculator only does Workspace memory, what's the plan for when we later fuse graph and workspace memory planning such that they could use the same memory pool? it seems like the workspace memory alignment should be a property of the function that allocates it, no?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] manupa-arm commented on a change in pull request #8019: [uTVM][AOT] Adding workspace byte alignment
Posted by GitBox <gi...@apache.org>.
manupa-arm commented on a change in pull request #8019:
URL: https://github.com/apache/tvm/pull/8019#discussion_r632009356
##########
File path: include/tvm/runtime/crt/stack_allocator.h
##########
@@ -45,14 +45,61 @@ typedef struct {
size_t workspace_size; // Total number of bytes in the workspace
} tvm_workspace_t;
+/*!
+ * \brief Initialize the stack-based memory manager
+ *
+ * \param tvm_runtime_workspace The tvm_workspace_t struct containing state
+ * \param g_aot_memory The memory buffer used to allocate within
+ * \param workspace_size The total size of the workspace buffer workspace
+ */
tvm_crt_error_t StackMemoryManager_Init(tvm_workspace_t* tvm_runtime_workspace,
uint8_t* g_aot_memory, size_t workspace_size);
+/*!
+ * \brief The intended user-facing function to allocate within the buffer. It wraps
+ * StackMemoryManager_Allocate_Body enable and disable the FIFO check that is useful for debugging
+ * the AoT codegen.
+ *
+ * \param tvm_runtime_workspace The tvm_workspace_t struct containing state
+ * \param nbytes The number of bytes required for the allocation
+ * \param current_alloc The pointer-to-pointer to be populated with the allocated address
+ */
tvm_crt_error_t StackMemoryManager_Allocate(tvm_workspace_t* tvm_runtime_workspace, int32_t nbytes,
- void**);
+ void** current_alloc);
+
+/*!
+ * \brief The internal function that accepts allocate inputs and an extra byte to say to enable the
+ * FIFO check that is useful in debugging for debugging the AoT codegen.
+ *
+ * \param tvm_runtime_workspace The tvm_workspace_t struct containing state
+ * \param nbytes The number of bytes required for the allocation
+ * \param current_alloc The pointer-to-pointer to be populated with the allocated address
+ * \param do_fifo_check THis indicates to perform a check FIFO pattern Allocs/Frees
Review comment:
Oh yes, good catch! corrected that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] areusch commented on a change in pull request #8019: [uTVM][AOT] Adding workspace byte alignment
Posted by GitBox <gi...@apache.org>.
areusch commented on a change in pull request #8019:
URL: https://github.com/apache/tvm/pull/8019#discussion_r632031281
##########
File path: src/tir/analysis/calculate_workspace.cc
##########
@@ -33,10 +33,12 @@ class WorkspaceCalculator : public StmtExprVisitor {
public:
WorkspaceCalculator() = default;
size_t operator()(const PrimFunc& func);
+ size_t byte_alignment = 1;
Review comment:
maybe this should become a constant somewhere just to make sure we use the correct default always. this isn't ideal--if Target was the correct place for this I'd just suggest we make it the default there. but perhaps this can be a stopgap til we fix Target
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org