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 2020/04/26 10:21:20 UTC
[GitHub] [incubator-tvm] liangfu commented on a change in pull request #5417: [RUNTIME][uTVM] AutoTVM + uTVM for Cortex-M7
liangfu commented on a change in pull request #5417:
URL: https://github.com/apache/incubator-tvm/pull/5417#discussion_r415270147
##########
File path: python/tvm/micro/device/base.py
##########
@@ -92,71 +95,142 @@ def create_micro_lib_base(
options : List[str]
additional options to pass to GCC
+
+ lib_src_paths : Optional[List[str]]
+ paths to additional source files to be compiled into the library
"""
+ # look at these (specifically `strip`):
+ # https://stackoverflow.com/questions/15314581/g-compiler-flag-to-minimize-binary-size
base_compile_cmd = [
- f"{toolchain_prefix}gcc",
- "-std=c11",
- "-Wall",
- "-Wextra",
- "--pedantic",
- "-c",
- "-O0",
- "-g",
- "-nostartfiles",
- "-nodefaultlibs",
- "-nostdlib",
- "-fdata-sections",
- "-ffunction-sections",
+ f'{toolchain_prefix}gcc',
Review comment:
Please undo the changes in converting double quotes to single quotes, and please undo other similar changes as well.
##########
File path: python/tvm/micro/device/arm/stm32f746xx.py
##########
@@ -36,23 +55,41 @@ def create_micro_lib(obj_path, src_path, lib_type, options=None):
options : Optional[List[str]]
additional options to pass to GCC
+
+ lib_src_paths : Optional[List[str]]
+ TODO
"""
if options is None:
options = []
+ else:
+ options = list(options)
+
options += [
- "-mcpu=cortex-m7",
- "-mlittle-endian",
- "-mfloat-abi=hard",
- "-mfpu=fpv5-sp-d16",
- "-mthumb",
- "-gdwarf-5",
+ # TODO(weberlo): make a debug flag
+ '-O2',
+ '-march=armv7e-m',
+ '-mcpu=cortex-m7',
+ '-mlittle-endian',
+ '-mfloat-abi=hard',
+ '-mfpu=fpv5-sp-d16',
+ '-mthumb',
+ '-ffast-math',
+ '-gdwarf-5',
+ '-DARM_MATH_CM7',
+ '-D__FPU_PRESENT=1U',
+ '-DARM_MATH_DSP',
+ '-Wno-unused-variable',
Review comment:
Is this really necessary? Why can't we remove the unused variables?
##########
File path: Makefile
##########
@@ -73,7 +73,10 @@ build/libtvm_web_runtime.js: build/libtvm_web_runtime.bc
cpplint:
python3 3rdparty/dmlc-core/scripts/lint.py vta cpp vta/include vta/src
python3 3rdparty/dmlc-core/scripts/lint.py topi cpp topi/include;
- python3 3rdparty/dmlc-core/scripts/lint.py tvm cpp include src \
+ # Note: exclude src/runtime/micro/host_driven becuase it contains C99 files.
+ python3 3rdparty/dmlc-core/scripts/lint.py tvm cpp \
+ --exclude_path=src/runtime/micro/host_driven \
Review comment:
Instead of making an exception here, can you try to locate the error and add inline exceptions ?
Here is an example:
```c
int b = 1;
float a = (float)b; // NOLINT(*)
```
`// NOLINT(*)` is used here to make an exception to avoid using `static_cast` in C++.
##########
File path: python/tvm/micro/device/base.py
##########
@@ -92,71 +95,142 @@ def create_micro_lib_base(
options : List[str]
additional options to pass to GCC
+
+ lib_src_paths : Optional[List[str]]
+ paths to additional source files to be compiled into the library
"""
+ # look at these (specifically `strip`):
+ # https://stackoverflow.com/questions/15314581/g-compiler-flag-to-minimize-binary-size
base_compile_cmd = [
- f"{toolchain_prefix}gcc",
- "-std=c11",
- "-Wall",
- "-Wextra",
- "--pedantic",
- "-c",
- "-O0",
- "-g",
- "-nostartfiles",
- "-nodefaultlibs",
- "-nostdlib",
- "-fdata-sections",
- "-ffunction-sections",
+ f'{toolchain_prefix}gcc',
+ '-std=c11',
+ '-Wall',
+ '-Wextra',
+ '--pedantic',
+ '-c',
+ '-g',
+ '-nostartfiles',
+ '-nodefaultlibs',
+ '-nostdlib',
+ '-fdata-sections',
+ '-ffunction-sections',
]
if options is not None:
base_compile_cmd += options
src_paths = []
include_paths = find_include_path() + [get_micro_host_driven_dir()]
tmp_dir = _util.tempdir()
- # we might transform the src path in one of the branches below
+ # we need to create a new src file in the operator branch
new_in_src_path = in_src_path
if lib_type == LibType.RUNTIME:
dev_dir = _get_device_source_dir(device_id)
- dev_src_paths = glob.glob(f"{dev_dir}/*.[csS]")
+
+ dev_src_paths = glob.glob(f'{dev_dir}/*.[csS]')
# there needs to at least be a utvm_timer.c file
assert dev_src_paths
- assert "utvm_timer.c" in map(os.path.basename, dev_src_paths)
+ assert 'utvm_timer.c' in map(os.path.basename, dev_src_paths)
+
src_paths += dev_src_paths
elif lib_type == LibType.OPERATOR:
- # create a temporary copy of the source, so we can inject the dev lib
+ # create a temporary copy of the operator source, so we can inject the dev lib
# header without modifying the original.
- temp_src_path = tmp_dir.relpath("temp.c")
- with open(in_src_path, "r") as f:
+ temp_src_path = tmp_dir.relpath('temp.c')
+ with open(in_src_path, 'r') as f:
src_lines = f.read().splitlines()
- src_lines.insert(0, "#include \"utvm_device_dylib_redirect.c\"")
- with open(temp_src_path, "w") as f:
- f.write("\n".join(src_lines))
+ src_lines.insert(0, '#include "utvm_device_dylib_redirect.c"')
+ with open(temp_src_path, 'w') as f:
+ f.write('\n'.join(src_lines))
new_in_src_path = temp_src_path
- base_compile_cmd += ["-c"]
else:
- raise RuntimeError("unknown lib type")
+ raise RuntimeError('unknown lib type')
src_paths += [new_in_src_path]
+ # add any src paths required by the operator
+ if lib_src_paths is not None:
+ src_paths += lib_src_paths
+
+ # print(f'include paths: {include_paths}')
for path in include_paths:
- base_compile_cmd += ["-I", path]
+ base_compile_cmd += ['-I', path]
Review comment:
Please undo the changes in converting double quotes to single quotes, and please undo other similar changes as well.
##########
File path: python/tvm/micro/device/host.py
##########
@@ -17,12 +17,26 @@
"""Compilation and config definitions for the host emulated device"""
import sys
-from . import create_micro_lib_base, register_device
+from . import create_micro_lib_base, register_device, gen_mem_layout, MemConstraint
-DEVICE_ID = "host"
-TOOLCHAIN_PREFIX = ""
+DEVICE_ID = 'host'
Review comment:
Please undo the changes in converting double quotes to single quotes, and please undo other similar changes as well.
##########
File path: src/runtime/micro/micro_common.h
##########
@@ -52,28 +53,115 @@ enum class SectionKind : size_t {
kNumKinds,
};
-/*! \brief union for storing values on varying target word sizes */
-union TargetVal {
- /*! \brief 32-bit pointer */
- uint32_t val32;
- /*! \brief 64-bit pointer */
- uint64_t val64;
+/*! \brief data type for word sizes */
+class TargetWordSize {
+ private:
+ size_t word_size_bits_;
Review comment:
Please put private members at the bottom of the class.
##########
File path: topi/python/topi/arm_cpu/cortex_m7/conv2d/direct.py
##########
@@ -0,0 +1,177 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+# pylint: disable=invalid-name
Review comment:
Please put this at the specific line of code, instead of leaving this on the top.
##########
File path: src/runtime/micro/micro_common.h
##########
@@ -52,28 +53,115 @@ enum class SectionKind : size_t {
kNumKinds,
};
-/*! \brief union for storing values on varying target word sizes */
-union TargetVal {
- /*! \brief 32-bit pointer */
- uint32_t val32;
- /*! \brief 64-bit pointer */
- uint64_t val64;
+/*! \brief data type for word sizes */
+class TargetWordSize {
+ private:
+ size_t word_size_bits_;
+
+ public:
+ explicit TargetWordSize(size_t word_size_bits) : word_size_bits_{word_size_bits} {
+ CHECK(word_size_bits == 32 || word_size_bits == 64)
+ << "only 32-bit and 64-bit are supported now";
+ }
+
+ size_t bytes() const {
+ return word_size_bits_ / 8;
+ }
+
+ size_t bits() const {
+ return word_size_bits_;
+ }
};
-/*! \brief absolute device address */
-class DevPtr {
+
+/*! \brief class for storing values on varying target word sizes */
+class TargetVal {
+ private:
+ size_t width_bits_;
+ uint64_t value_;
+
public:
- /*! \brief construct a device address with value `value` */
- explicit DevPtr(std::uintptr_t value) : value_(TargetVal { .val64 = value }) {}
+ /*! \brief construct a TargetVal matching the size of the given integral argument */
+ template<typename T, typename U = typename std::enable_if<std::is_integral<T>::value, T>::type>
+ explicit constexpr TargetVal(T value) : TargetVal(sizeof(T) * 8, value) {}
+
+ /*! \brief construct an uninitialized value */
+ TargetVal() : width_bits_{0}, value_{0} {}
+
+ /*! \brief construct a TargetVal with explicit size and value */
+ TargetVal(size_t width_bits, uint64_t value) : width_bits_{width_bits} {
+ CHECK(width_bits >= 8 &&
+ width_bits <= 64 &&
+ (width_bits & (width_bits - 1)) == 0)
+ << "width_bits must be a power of 2 in [8, 64], got " << width_bits;
+ value_ = value & bitmask();
+ }
+
+ bool is_initialized() const { return width_bits_ != 0; }
Review comment:
Please use CamelCase for function names.
##########
File path: src/runtime/micro/host_driven/utvm_runtime.h
##########
@@ -46,20 +62,38 @@ typedef struct {
int32_t num_args;
} UTVMTask;
+/*!
+ * \brief TODO
+ */
extern void UTVMInit();
-extern void UTVMTimerReset();
-
+/*!
+ * \brief TODO
+ */
extern int32_t UTVMTimerStart();
-extern void UTVMTimerStop();
-
-extern uint32_t UTVMTimerRead();
+/*!
+ * \brief TODO
+ */
+extern uint32_t UTVMTimerStop(int32_t* err);
+/*!
+ * \brief TODO
+ */
void UTVMMain();
+/*!
+ * \brief TODO
Review comment:
Please leave a meaningful comment.
##########
File path: src/target/source/codegen_c_host.cc
##########
@@ -20,10 +20,10 @@
/*!
* \file codegen_c_host.cc
*/
-#include <tvm/target/codegen.h>
+#include "codegen_c_host.h"
Review comment:
Please put local headers on the bottom.
Suggested include sequence:
1. #include<tvm_header>
2. #include<std_header>
3. #include"local_header"
##########
File path: topi/python/topi/arm_cpu/cortex_m7/conv2d/direct.py
##########
@@ -0,0 +1,177 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+# pylint: disable=invalid-name
+"""Direct implementation of conv2d."""
+
+import tvm
+from tvm import autotvm
+from tvm.autotvm.task import deserialize_args
+from topi.nn.conv2d import conv2d_nchw, conv2d_nhwc
+from topi.util import get_const_tuple, get_const_int, traverse_inline
+
+def conv2d_direct(*args, **kwargs):
+ """Schedule function for directly-scheduled conv2d."""
+ assert not kwargs, "Do not support kwargs in template function call"
+ args = deserialize_args(args)
+ data, kernel = args[:2]
+ layout = args[-2]
+ cfg = autotvm.get_config()
+ args = [cfg] + args
+ conv = conv2d_direct_compute(*args)
+ if layout == 'NHWC':
+ sched = conv2d_direct_nhwc_schedule(cfg, [data, kernel, conv])
+ elif layout == 'NCHW':
+ sched = conv2d_direct_nchw_schedule(cfg, [data, kernel, conv])
+ else:
+ raise RuntimeError(f'unsupported data layout "{layout}"')
+ return sched, [data, kernel, conv]
+
+
+conv2d_direct.template_key = 'direct'
+conv2d_direct.default_data_layout = 'NHWC'
+conv2d_direct.default_kernel_layout = 'HWIO'
+
+@autotvm.register_topi_compute('conv2d_direct.micro_dev')
+def conv2d_direct_compute(*args):
+ layout = args[-2]
+ if layout == 'NHWC':
+ return _conv2d_direct_nhwc_compute(*args)
+ if layout == 'NCHW':
+ return _conv2d_direct_nchw_compute(*args)
+
+ raise RuntimeError(f'unsupported data layout "{layout}"')
+
+
+def _conv2d_direct_nhwc_compute(cfg, data, kernel, strides, padding, dilation, layout, out_dtype):
+ assert layout == 'NHWC'
+ conv = conv2d_nhwc(data, kernel, strides, padding, dilation, out_dtype)
+
+ ###########################
+ # Config Space Definition #
+ ###########################
Review comment:
Can we make this comment more concise (by removing the extra decoration) ?
----------------------------------------------------------------
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