areusch commented on a change in pull request #7785:
URL: https://github.com/apache/tvm/pull/7785#discussion_r619868182
##########
File path: src/runtime/crt/memory/stack_allocator.c
##########
@@ -16,17 +16,22 @@
* specific language governing permissions and limitations
* under the License.
*/
-
// LINT_C_FILE
-
#include <tvm/runtime/crt/stack_allocator.h>
+#ifdef TVM_CRT_DEBUG
+#include <tvm/runtime/crt/logging.h>
+#endif
void* StackMemoryManager_Allocate(tvm_workspace_t* tvm_runtime_workspace,
int32_t nbytes) {
uint32_t offset_bytes = (~nbytes + 1) & (TVM_RUNTIME_ALLOC_ALIGNMENT_BYTES -
1);
uint8_t* current_alloc = tvm_runtime_workspace->next_alloc;
uint8_t* next_alloc = tvm_runtime_workspace->next_alloc + nbytes +
offset_bytes;
uint8_t* workspace_end = tvm_runtime_workspace->workspace +
tvm_runtime_workspace->workspace_size;
-
+#ifdef TVM_CRT_DEBUG
Review comment:
ah i'm sorry--my previous comment was incorrect. you're indeed right
that this checking mode wastes 4 bytes per tensor. I understand that we
especially need to be cognizant of the alignment issue, so how about this
proposal then:
- the libmemory.a used in `make crttest` is built with the `crt_config.h`
used to build `standalone_crt` for:
1. the host machine
2. tests launched from `test_crt.py`
- in these cases, we can afford the extra cycles for a check, and I think
this shouldn't affect test results since we aren't using accelerators from here.
- so my proposal is: let's include this check by defining a flag in
`src/runtime/crt/host/crt_config.h`. we can also include a commented copy of
this flag in the template `crt_config.h`, but leave it off there. finally, i'm
not sure we should use `TVM_CRT_DEBUG`, since that may be spammy in unit tests;
but perhaps we could define another flag?
let me know how this sounds to you. the main thing i'd like to achieve is
that unit tests of the CRT and of the AOT that are executed in CI are run with
this check enabled.
btw, we should probably discuss moving the alignment rules to a higher level
than the runtime memory allocator. I think it's fine to leave it there for this
PR, but seems like it should be part of our higher-level memory planning work.
--
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:
us...@infra.apache.org
