[GitHub] [tvm] echuraev commented on a diff in pull request #13349: [TIR][Hexagon] Add vtcm memory capacity verification for Hexagon target

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

echuraev commented on code in PR #13349:
URL: https://github.com/apache/tvm/pull/13349#discussion_r1020134014


##########
include/tvm/target/utils.h:
##########
@@ -0,0 +1,46 @@
+/*
+ * 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.
+ */
+
+/*!
+ * \file target/utils.h
+ * \brief Common utilities.
+ */
+
+#ifndef TVM_TARGET_UTILS_H_
+#define TVM_TARGET_UTILS_H_
+
+#include <tvm/ir/expr.h>
+#include <tvm/runtime/container/optional.h>
+#include <tvm/target/target.h>
+
+namespace tvm {
+
+/*! \brief Extract attribute from a target. */
+inline Integer Extract(const Target& target, const char* name) {
+  ICHECK(target.defined());
+  if (Optional<Integer> v = target->GetAttr<Integer>(name)) {
+    return v.value();
+  }
+  LOG(FATAL) << "AttributedError: \"" << name << "\" is not defined in the target";
+  throw;

Review Comment:
   As far as I remember, `LOG(FATAL)` should throw an exception. So, we don't need to add `throw` after that. If there is a compilation warning about return value, probably we could do something like that:
   ```suggestion
     Optional<Integer> v = target->GetAttr<Integer>(name);
     ICHECK(v != nullptr) << "AttributedError: \"" << name << "\" is not defined in the target";
     return v.value();
   ```



##########
python/tvm/driver/build_module.py:
##########
@@ -221,29 +221,30 @@ def build(
     ----
     See the note on :any:`tvm.target` on target string format.
     """
-    if isinstance(inputs, te.Schedule):
-        if args is None:
-            raise ValueError("args must be given for build from schedule")
-        input_mod = lower(inputs, args, name=name, binds=binds)
-    elif isinstance(inputs, (list, tuple, container.Array)):
-        merged_mod = tvm.IRModule({})
-        for x in inputs:
-            merged_mod.update(lower(x))
-        input_mod = merged_mod
-    elif isinstance(inputs, PrimFunc):
-        input_mod = lower(inputs, name=name)
-    elif isinstance(inputs, tvm.IRModule):
-        input_mod = lower(inputs)
-    elif not isinstance(inputs, (dict, container.Map)):
-        raise ValueError(
-            f"Inputs must be te.Schedule, IRModule, PrimFunc, "
-            f"or dict of target to IRModule, "
-            f"but got {type(inputs)}."
-        )
+    target = Target.current() if target is None else target
+    target = target if target else "llvm"
+    with target:

Review Comment:
   Sorry, could you please explain, why do we need this target context and these changes?



##########
include/tvm/target/utils.h:
##########
@@ -0,0 +1,46 @@
+/*
+ * 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.
+ */
+
+/*!
+ * \file target/utils.h
+ * \brief Common utilities.
+ */
+
+#ifndef TVM_TARGET_UTILS_H_
+#define TVM_TARGET_UTILS_H_
+
+#include <tvm/ir/expr.h>
+#include <tvm/runtime/container/optional.h>
+#include <tvm/target/target.h>
+
+namespace tvm {
+
+/*! \brief Extract attribute from a target. */
+inline Integer Extract(const Target& target, const char* name) {

Review Comment:
   The extracted attributes might have different types, not only `Integer`. Probably we can do this function as a template? Also, have some doubts about do we really need this helper function?



##########
python/tvm/autotvm/measure/measure_methods.py:
##########
@@ -493,7 +495,9 @@ def set_task(self, task):
         return server, tracker
 
 
-def _build_func_common(measure_input, runtime=None, check_gpu=None, build_option=None):
+def _build_func_common(
+    measure_input, runtime=None, check_gpu=None, build_option=None, check_hexagon=None

Review Comment:
   Probably we could pass all check arguments (such as `check_gpu` and `check_hexagon`) in a dictionary or list? In this case it won't be necessary to change function signature every time when we want to add new check



-- 
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