gemini-code-assist[bot] commented on code in PR #19774: URL: https://github.com/apache/tvm/pull/19774#discussion_r3410722869
########## python/tvm/support/emcc.py: ########## @@ -21,8 +21,45 @@ import subprocess from pathlib import Path -from tvm.base import py_str -from tvm.libinfo import find_lib_path +from tvm import libinfo Review Comment:  The `os` module is used in `find_wasm_lib` (e.g., `os.environ` and `os.path.join`), but it is not imported in this file. This will cause a `NameError` at runtime. Please add `import os` to the imports. ```suggestion import os import subprocess from pathlib import Path from tvm import libinfo ``` ########## python/tvm/relax/frontend/torch/base_fx_graph_translator.py: ########## @@ -391,6 +391,17 @@ def _log_softmax(self, node: fx.Node) -> relax.Var: dim = node.args[1] if len(node.args) > 1 else node.kwargs.get("dim", -1) return self.block_builder.emit(relax.op.nn.log_softmax(x, dim)) + def _logical_and(self, node: fx.Node) -> relax.Var: + lhs = self.env[node.args[0]] + rhs = self.env[node.args[1]] + # torch.logical_and accepts any dtype (treating nonzero as True) and returns bool, but + # relax.op.logical_and requires boolean inputs, so cast non-bool inputs to bool first. + if lhs.struct_info.dtype != "bool": + lhs = self.block_builder.emit(relax.op.astype(lhs, "bool")) + if rhs.struct_info.dtype != "bool": + rhs = self.block_builder.emit(relax.op.astype(rhs, "bool")) Review Comment:  Accessing `lhs.struct_info.dtype` directly is risky because `struct_info` can be `None` or may not have a `dtype` attribute (e.g., if it is a `TupleStructInfo` or `ObjectStructInfo`). Use `getattr` to safely retrieve the `dtype` and avoid potential `AttributeError`s. ```suggestion if getattr(lhs.struct_info, "dtype", None) != "bool": lhs = self.block_builder.emit(relax.op.astype(lhs, "bool")) if getattr(rhs.struct_info, "dtype", None) != "bool": rhs = self.block_builder.emit(relax.op.astype(rhs, "bool")) ``` ########## python/tvm/s_tir/dlight/gpu/fallback.py: ########## @@ -26,6 +26,24 @@ from .base import GPUScheduleRule +def _has_internal_thread_env(stmt: tirx.Stmt) -> bool: + """Check whether a statement already launches GPU threads internally, + e.g. via `T.launch_thread` (AttrStmt "thread_extent") or nested + thread-bound loops. Such blocks manage their own thread environment + and must not be wrapped in an additional thread binding.""" + found = False + + def _visit(node): + nonlocal found + if isinstance(node, tirx.AttrStmt) and node.attr_key in ("thread_extent", "virtual_thread"): + found = True + elif isinstance(node, tirx.For) and node.kind == tirx.ForKind.THREAD_BINDING: + found = True Review Comment:  The `_visit` helper function traverses the entire AST of the statement. Since we only need to know if *any* internal thread environment exists, we can optimize this by returning early once `found` is set to `True`, avoiding unnecessary traversal of the remaining nodes in large ASTs. ```suggestion def _visit(node): nonlocal found if found: return if isinstance(node, tirx.AttrStmt) and node.attr_key in ("thread_extent", "virtual_thread"): found = True elif isinstance(node, tirx.For) and node.kind == tirx.ForKind.THREAD_BINDING: found = True ``` ########## python/tvm/_autoload_backends.py: ########## @@ -0,0 +1,50 @@ +# 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. +"""Autoload out-of-tree backends registered via ``tvm.backends`` entry points. + +Out-of-tree extensions opt into being loaded automatically at ``import tvm`` +time by declaring an entry point in the ``tvm.backends`` group:: + + [project.entry-points."tvm.backends"] + tvm_foo = "tvm_foo:_autoload" + +Autoload can be disabled via ``TVM_DEVICE_BACKEND_AUTOLOAD=0``. +""" + +import os +import warnings +from importlib.metadata import entry_points + +# Guard so autoload runs at most once per process, even if invoked again. +_AUTO_LOAD_DONE = False + + +def _autoload_backends(): + """Discover and invoke out-of-tree backends registered via entry points.""" + global _AUTO_LOAD_DONE + if _AUTO_LOAD_DONE: + return + _AUTO_LOAD_DONE = True + + if os.environ.get("TVM_DEVICE_BACKEND_AUTOLOAD", "1") == "0": + return + + for entry_pt in entry_points(group="tvm.backends"): Review Comment:  `importlib.metadata.entry_points()` does not accept the `group` keyword argument in Python 3.9, which is the minimum supported Python version for TVM. Calling it with `group="tvm.backends"` will raise a `TypeError` on Python 3.9. To maintain compatibility, check the Python version or use a fallback for Python 3.9. ```suggestion import sys if sys.version_info >= (3, 10): backends = entry_points(group="tvm.backends") else: backends = entry_points().get("tvm.backends", []) for entry_pt in backends: ``` ########## python/tvm/relax/frontend/torch/base_fx_graph_translator.py: ########## @@ -1673,6 +1684,20 @@ def _sum(self, node: fx.Node) -> relax.Var: if isinstance(dim, list | tuple) and len(dim) == 0: dim = None keepdim = args[2] if len(node.args) > 2 else node.kwargs.get("keepdim", False) + dtype = node.kwargs.get("dtype", None) + if dtype is not None: + x = self.block_builder.emit( + relax.op.astype(x, self._convert_data_type(dtype, self.env)) + ) + else: + # Match PyTorch type promotion: summing bool or integer tensors + # accumulates in int64 unless an explicit dtype is given. + input_dtype = x.struct_info.dtype + if input_dtype == "bool" or ( + (input_dtype.startswith("int") or input_dtype.startswith("uint")) + and input_dtype != "int64" + ): Review Comment:  Accessing `x.struct_info.dtype` directly can raise an `AttributeError` if `struct_info` is `None` or is not a `TensorStructInfo`. Use `getattr` to safely access the `dtype` property. ```suggestion input_dtype = getattr(x.struct_info, "dtype", None) if input_dtype is not None and ( input_dtype == "bool" or ( (input_dtype.startswith("int") or input_dtype.startswith("uint")) and input_dtype != "int64" ) ): ``` ########## python/tvm/contrib/hexagon/pytest_plugin.py: ########## @@ -105,13 +112,18 @@ def get_free_port() -> int: if port > LISTEN_PORT_MAX: port = LISTEN_PORT_MIN - while tvm.contrib.hexagon.build._is_port_in_use(port): + while _is_port_in_use(port): port = port + 1 if port < LISTEN_PORT_MAX else LISTEN_PORT_MIN PREVIOUS_PORT = port return port +def _is_port_in_use(port: int) -> bool: + with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as sock: + return sock.connect_ex(("localhost", port)) == 0 Review Comment:  Using `"localhost"` with `socket.AF_INET` (which is strictly IPv4) can lead to resolution issues or delays on systems where `localhost` resolves to IPv6 (`::1`) first or is misconfigured. It is safer and more robust to use the explicit IPv4 loopback address `"127.0.0.1"`. ```suggestion return sock.connect_ex(("127.0.0.1", port)) == 0 ``` -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
