cconvey commented on code in PR #13256:
URL: https://github.com/apache/tvm/pull/13256#discussion_r1034756650
##########
cmake/modules/Hexagon.cmake:
##########
@@ -178,6 +178,15 @@ if(BUILD_FOR_HEXAGON)
"${TVMRT_SOURCE_DIR}/hexagon/ops/*.cc"
)
+ include_directories(
+ "${TVMRT_SOURCE_DIR}/hexagon/ops"
+ )
+
+ set_source_files_properties(
+ "${TVMRT_SOURCE_DIR}/hexagon/ops/conv2d_quant_hvx.cc"
+ PROPERTIES COMPILE_FLAGS "-mhvx"
Review Comment:
Are we confident that `-mhvx` is supported by all of the compilers that
might build this code?
I'm assuming that _typically_ the clang provided by Hexagon Toolchain will
be used. But I'm a little fuzzy about the intended level of support for other
compilers, e.g. a user-supplied build of Clang/LLVM.
##########
src/runtime/hexagon/ops/conv2d.h:
##########
@@ -144,7 +207,42 @@ void blockize_hwc_16b(void* out, void* inp_flat, int
height, int width, int dept
* @param width
* @param depth
*/
-void deblockize_hwc_16b(void* out_flat, void* inp, int height, int width, int
depth);
+template <typename T, int block_height, int block_width, int block_depth>
+void deblockize_hwc(void* out_flat, void* inp, int height, int width, int
depth) {
Review Comment:
Would it make sense for the type of `inp` to be `const void*`?
##########
src/runtime/hexagon/ops/conv2d.h:
##########
@@ -133,7 +155,48 @@ inline uintptr_t hwio_at(const DLTensor& f, int y, int x,
int i, int o) {
* @param width
* @param depth
*/
-void blockize_hwc_16b(void* out, void* inp_flat, int height, int width, int
depth);
+template <typename T, int block_height, int block_width, int block_depth>
+void blockize_hwc(void* out, void* inp_flat, int height, int width, int depth)
{
Review Comment:
Would it make sense for `inp_flat`'s type to be `const void*` rather than
`void*`?
This is probably a bit of a stylistic choice; I just figured I'd ask.
##########
cmake/modules/Hexagon.cmake:
##########
@@ -178,6 +178,15 @@ if(BUILD_FOR_HEXAGON)
"${TVMRT_SOURCE_DIR}/hexagon/ops/*.cc"
)
+ include_directories(
+ "${TVMRT_SOURCE_DIR}/hexagon/ops"
+ )
+
+ set_source_files_properties(
+ "${TVMRT_SOURCE_DIR}/hexagon/ops/conv2d_quant_hvx.cc"
+ PROPERTIES COMPILE_FLAGS "-mhvx"
Review Comment:
Would it make sense to update
[src/runtime/hexagon/README.md](https://github.com/apache/tvm/blob/15ee9bb5757915c73569f3ebdb5e52a4312663aa/src/runtime/hexagon/README.md)
to clarify the version(s) of LLVM that support flags like `-mhvx`?
Or alternatively, use CMake's
[CheckCXXCompilerFlag](https://cmake.org/cmake/help/latest/module/CheckCXXCompilerFlag.html)
function to see if `-mhvx` is supported, and only use that flag if it is?
##########
src/runtime/hexagon/ops/conv2d.h:
##########
@@ -75,15 +77,31 @@ inline void* to_ptr(uintptr_t v) { return
reinterpret_cast<void*>(v); }
inline uintptr_t to_uint(void* ptr) { return reinterpret_cast<uintptr_t>(ptr);
}
-constexpr int xyc_to_sm_16b(int y, int x, int c) {
+inline constexpr int yxc_to_sm_16b(int y, int x, int c) {
// Map y,x,c coordinates within a block to the offset (in 16-bit elements)
// from the beginning of the block in spatial-major layout.
// 10-bit spatial mask: yyyxcccccx
assert(y >= 0 && x >= 0 && c >= 0);
return y << 7 | (x & 2) << 5 | c << 1 | (x & 1);
}
-constexpr int hwio_to_sm_16b(int width, int y, int x, int i, int o) {
+inline constexpr int yxc_to_sm_8b(int y, int x, int c) {
+ // Map y,x,c coordinates within a block to the offset (in 8-bit elements)
+ // from the beginning of the block in spatial-major layout.
+ // 10-bit spatial mask: yyyxxxccccc
Review Comment:
I'm pretty sure we can rely on `assert` being disabled when
`CMAKE_BUILD_TYPE=Release`. See
https://stackoverflow.com/questions/34302265/does-cmake-build-type-release-imply-dndebug.
--
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-unsubscr...@tvm.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
