[GitHub] [tvm] cconvey commented on a diff in pull request #13256: [Hexagon] Add HVX quant conv2d implementation

[GitBox <gi...@apache.org>]

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-unsubscribe@tvm.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org