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/10/12 09:58:56 UTC
[GitHub] [tvm] PhilippvK opened a new pull request #9255: Fix inconsistencies in graph_executor function names handling
PhilippvK opened a new pull request #9255:
URL: https://github.com/apache/tvm/pull/9255
Overview of changes:
- Updates value of `TVM_CRT_MAX_STRLEN_FUNCTION_NAME` from `80` to `120`
- Replaces all occurences of `[120]` with `[TVM_CRT_MAX_STRLEN_FUNCTION_NAME]` to maintain consistency and make the array lengths user-configurable.
- Introduces `TVM_CRT_MAX_STRLEN_PARAM_NAME` used for parameter names only
- Adds comments to `kMaxFuncNameLength` variabe in src/relay/backend/te_compiler_cache.cc making sure that the values are kept "in sync". (sort of)
See Issue #8953 for more context. The actual bug reported there however can only be fixed by increasing the TVM_CRT_MAX_STRLEN_FUNCTION_NAME to `121` as demonstrated here:
https://github.com/PhilippvK/tvm/commits/bug-demo-fixed
---
CC @areusch
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] PhilippvK commented on pull request #9255: Fix inconsistencies in graph_executor function names handling
Posted by GitBox <gi...@apache.org>.
PhilippvK commented on pull request #9255:
URL: https://github.com/apache/tvm/pull/9255#issuecomment-950264721
@masahi Conflicts fixed
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] masahi commented on pull request #9255: Fix inconsistencies in graph_executor function names handling
Posted by GitBox <gi...@apache.org>.
masahi commented on pull request #9255:
URL: https://github.com/apache/tvm/pull/9255#issuecomment-950276356
Thanks @PhilippvK @areusch @Mousius
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] masahi commented on pull request #9255: Fix inconsistencies in graph_executor function names handling
Posted by GitBox <gi...@apache.org>.
masahi commented on pull request #9255:
URL: https://github.com/apache/tvm/pull/9255#issuecomment-949964836
@PhilippvK please fix the conflict.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] PhilippvK commented on pull request #9255: Fix inconsistencies in graph_executor function names handling
Posted by GitBox <gi...@apache.org>.
PhilippvK commented on pull request #9255:
URL: https://github.com/apache/tvm/pull/9255#issuecomment-943468171
@areusch CI should be fixed now
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] areusch commented on a change in pull request #9255: Fix inconsistencies in graph_executor function names handling
Posted by GitBox <gi...@apache.org>.
areusch commented on a change in pull request #9255:
URL: https://github.com/apache/tvm/pull/9255#discussion_r728621033
##########
File path: src/runtime/crt/crt_config-template.h
##########
@@ -49,7 +49,10 @@
#define TVM_CRT_MAX_STRLEN_DLTYPE 10
/*! Maximum supported string length in function names */
-#define TVM_CRT_MAX_STRLEN_FUNCTION_NAME 80
+#define TVM_CRT_MAX_STRLEN_FUNCTION_NAME 120
+
+/*! Maximum supported string length in parameter names */
+#define TVM_CRT_MAX_STRLEN_PARAM_NAME 80
Review comment:
i think the main thing is that it's configurable in `crt_config.h` and we pick a sane default. we can consider a sane default to:
1. fit most parameters and inputs/outputs
2. not be so excessively large as to take too much memory
I guess 80 was chosen before (i inherited this code so am not sure why). it seems like we could choose something smaller e.g. 20, but on the other hand it's configurable now and by not changing things, this PR is just a cleanup PR. so happy to merge as-is.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] PhilippvK commented on pull request #9255: Fix inconsistencies in graph_executor function names handling
Posted by GitBox <gi...@apache.org>.
PhilippvK commented on pull request #9255:
URL: https://github.com/apache/tvm/pull/9255#issuecomment-940857620
I used `80` as default value for `TVM_CRT_MAX_STRLEN_PARAM_NAME` which is probably quite high. Please let me know if I should decrease it to something else.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] masahi merged pull request #9255: Fix inconsistencies in graph_executor function names handling
Posted by GitBox <gi...@apache.org>.
masahi merged pull request #9255:
URL: https://github.com/apache/tvm/pull/9255
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org