gemini-code-assist[bot] commented on code in PR #18847:
URL: https://github.com/apache/tvm/pull/18847#discussion_r2867455490
##########
python/tvm/relax/op/base.py:
##########
@@ -76,7 +76,9 @@ def _wrap_inline_arg_tuple(args) -> Expr:
in-line relax Tuple.
"""
- if (
+ if isinstance(args, tuple | list):
+ return tvm.relax.Tuple([convert_to_expr(a) if not isinstance(a, Expr)
else a for a in args])
Review Comment:
The list comprehension can be simplified. Since `convert_to_expr` is
idempotent for `relax.Expr` inputs (it just returns the input), the `if not
isinstance(a, Expr)` check is redundant. You can simplify this to just
`tvm.relax.Tuple([convert_to_expr(a) for a in args])` for better readability
and conciseness.
```suggestion
return tvm.relax.Tuple([convert_to_expr(a) for a in args])
```
##########
python/tvm/relax/op/base.py:
##########
@@ -731,6 +729,10 @@ def call_inplace_packed(
func = func.global_symbol
op = ExternFunc(func)
+ args = tuple(
+ convert_to_expr(a) if isinstance(a, int | float | str | tuple | list)
else a
+ for a in args
+ )
Review Comment:
The argument conversion logic here is incomplete. The condition
`isinstance(a, int | float | str | tuple | list)` will not be true for
`tir.PrimExpr` arguments, which will then be passed without conversion.
However, `convert_to_expr` is designed to handle `PrimExpr` by wrapping it in a
`PrimValue`. The downstream FFI function expects a tuple of `Expr`.
To ensure all convertible types are handled correctly and to simplify the
code, you can apply `convert_to_expr` to all arguments unconditionally. The
function is idempotent for `Expr` inputs.
```python
args = tuple(convert_to_expr(a) for a in args)
```
##########
python/tvm/relax/op/index.py:
##########
@@ -101,6 +100,11 @@ def strided_slice(
strided_slice require the input `begin`, `end` and `strides` to have the
same length as `axes`.
"""
+ axes = convert_to_expr(axes) if not isinstance(axes, Expr) else axes
+ begin = convert_to_expr(begin) if not isinstance(begin, Expr) else begin
+ end = convert_to_expr(end) if not isinstance(end, Expr) else end
+ if strides is not None and not isinstance(strides, Expr):
+ strides = convert_to_expr(strides)
Review Comment:
The checks `if not isinstance(..., Expr)` are redundant because
`convert_to_expr` is idempotent on `Expr` inputs. You can simplify this block
of code by applying `convert_to_expr` unconditionally.
```suggestion
axes = convert_to_expr(axes)
begin = convert_to_expr(begin)
end = convert_to_expr(end)
if strides is not None:
strides = convert_to_expr(strides)
```
##########
python/tvm/script/ir_builder/relax/ir.py:
##########
@@ -485,6 +488,9 @@ def call_py_func(
call: Call
The created Relax Call for call_py_func operator.
"""
+ args = py_tuple(
+ convert_to_expr(a) if isinstance(a, _CONVERTIBLE_TYPES) else a for a
in args
+ )
Review Comment:
Similar to `call_packed`, the argument conversion logic here is incomplete.
It doesn't handle `tir.PrimExpr` because of the `isinstance` check with
`_CONVERTIBLE_TYPES`. This could lead to runtime errors if a `PrimExpr` is
passed as an argument.
You can simplify this and ensure correctness by applying `convert_to_expr`
to all arguments.
```python
args = py_tuple(convert_to_expr(a) for a in args)
```
##########
python/tvm/script/ir_builder/relax/distributed/ir.py:
##########
@@ -82,7 +80,9 @@ def call_tir(
if isinstance(func, str):
func = ExternFunc(func)
- if isinstance(args, Expr) and not isinstance(args, RxTuple): # type:
ignore
+ if isinstance(args, tuple | list):
+ args = RxTuple(list(args))
Review Comment:
When `args` is a `tuple` or `list`, its elements are not being converted to
`relax.Expr`. This was previously handled by `@args_converter.auto` and its
removal requires explicit conversion. This could lead to errors if the list
contains non-`Expr` values.
Please convert each element to an `Expr`. You'll need to import
`convert_to_expr` from `tvm.relax.utils`.
```suggestion
args = RxTuple([convert_to_expr(a) for a in args])
```
##########
python/tvm/script/ir_builder/relax/ir.py:
##########
@@ -428,6 +429,9 @@ def call_packed(
The created Relax Call
"""
op = ExternFunc(func)
+ args = py_tuple(
+ convert_to_expr(a) if isinstance(a, _CONVERTIBLE_TYPES) else a for a
in args
+ )
Review Comment:
The argument conversion logic here is incomplete. The condition
`isinstance(a, _CONVERTIBLE_TYPES)` will not be true for `tir.PrimExpr`
arguments, which will then be passed without conversion. However,
`convert_to_expr` is designed to handle `PrimExpr`.
To ensure all convertible types are handled correctly and to simplify the
code, you can apply `convert_to_expr` to all arguments unconditionally.
```python
args = py_tuple(convert_to_expr(a) for a in args)
```
##########
python/tvm/relax/op/base.py:
##########
@@ -782,6 +783,10 @@ def call_pure_packed(
func = func.global_symbol
op = ExternFunc(func)
+ args = tuple(
+ convert_to_expr(a) if isinstance(a, int | float | str | tuple | list)
else a
+ for a in args
+ )
Review Comment:
Similar to `call_inplace_packed`, the argument conversion logic here is
incomplete. It doesn't handle `tir.PrimExpr` because of the `isinstance` check.
This could lead to runtime errors if a `PrimExpr` is passed as an argument.
You can simplify this and ensure correctness by applying `convert_to_expr`
to all arguments.
```python
args = tuple(convert_to_expr(a) for a in args)
```
##########
python/tvm/relax/op/distributed/distributed.py:
##########
@@ -99,7 +96,9 @@ def call_tir_local_view(
ret: Call
A call node for the call_tir_local_view operator.
"""
- if isinstance(args, Expr) and not isinstance(args, RxTuple): # type:
ignore
+ if isinstance(args, tuple | list):
+ args = RxTuple(list(args))
Review Comment:
The elements of `args` are not being converted to `relax.Expr` here. This
can cause issues if `args` contains non-`Expr` values. Please convert them, for
example using a list comprehension. You will also need to import
`convert_to_expr` from `...utils`.
```suggestion
args = RxTuple([convert_to_expr(a) for a in args])
```
##########
python/tvm/s_tir/schedule/_type_checker.py:
##########
@@ -345,17 +345,22 @@ def _type_check(v: Any, name: str, type_: Any) -> str |
None:
def type_checked(func: FType) -> FType:
"""Type check the input arguments of a function."""
sig = inspect.signature(func)
+ try:
+ hints = typing.get_type_hints(func)
+ except Exception:
+ hints = {}
Review Comment:
Catching a broad `Exception` can hide unexpected bugs.
`typing.get_type_hints` is documented to raise `TypeError` or `NameError` on
failure. It would be safer to catch these specific exceptions. If
`get_type_hints` fails for other reasons, it might indicate a problem that
shouldn't be silently ignored.
```suggestion
try:
hints = typing.get_type_hints(func)
except (TypeError, NameError):
hints = {}
```
--
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]
