gemini-code-assist[bot] commented on code in PR #19891:
URL: https://github.com/apache/tvm/pull/19891#discussion_r3476683681
##########
python/tvm/relax/frontend/onnx/onnx_frontend.py:
##########
@@ -441,7 +441,7 @@ def _impl_v1(cls, bb, inputs, attr, params):
def _to_numpy(x):
- if isinstance(x, relax.PrimValue):
+ if isinstance(x, tvm.tirx.PrimExpr):
x = x.value
if isinstance(x, tirx.IntImm | tirx.FloatImm):
x = x.value
Review Comment:
In `_to_numpy`, `x = x.value` is called directly on `tvm.tirx.PrimExpr`.
However, `PrimExpr` does not have a `.value` attribute (only `IntImm` and
`FloatImm` do). This will raise an `AttributeError` for any non-constant
`PrimExpr` (like `Var`), and is redundant/broken for constant `PrimExpr`s.
We should check if `x` is an `IntImm` or `FloatImm` before accessing
`.value`.
```suggestion
if isinstance(x, tvm.tirx.PrimExpr):
if isinstance(x, tirx.IntImm | tirx.FloatImm):
return x.value
return x
```
##########
python/tvm/relax/frontend/onnx/onnx_frontend.py:
##########
@@ -280,7 +280,7 @@ def get_prim_expr_list(
return np_value.tolist()
elif isinstance(inputs, relax.ShapeExpr):
return inputs.values
- elif isinstance(inputs, relax.PrimValue):
+ elif isinstance(inputs, tvm.tirx.PrimExpr):
return [inputs.value.value]
Review Comment:
Since `inputs` is already a `tvm.tirx.PrimExpr`, accessing `inputs.value`
will raise an `AttributeError` (as `PrimExpr` does not have a `.value`
attribute unless it is an `IntImm` or `FloatImm`). Since the function
`get_prim_expr_list` is intended to return a list of `PrimExpr`s, we should
return `[inputs]` directly.
```suggestion
elif isinstance(inputs, tvm.tirx.PrimExpr):
return [inputs]
```
##########
python/tvm/relax/frontend/torch/base_fx_graph_translator.py:
##########
@@ -2142,19 +2150,19 @@ def _adjust(val):
return input_shape[axis]
return val
- if isinstance(bound, relax.PrimValue):
+ if isinstance(bound, tirx.PrimExpr):
value = _adjust(bound.value)
- return relax.PrimValue(value)
+ return relax.prim_value(value)
Review Comment:
Since `bound` is already a `tirx.PrimExpr`, accessing `bound.value` will
raise an `AttributeError`. We should pass `bound` directly to `_adjust`.
```suggestion
if isinstance(bound, tirx.PrimExpr):
value = _adjust(bound)
return relax.prim_value(value)
```
##########
python/tvm/ir/expr.py:
##########
@@ -67,30 +62,29 @@ class GlobalVar(RelaxExpr):
def __init__(self, name_hint: str):
self.__init_handle_by_constructor__(_ffi_api.GlobalVar, name_hint)
- def __call__(self, *args: RelaxExpr) -> BaseExpr:
+ def __call__(self, *args: Expr) -> Expr:
"""Call the global variable.
Parameters
----------
- args: List[RelaxExpr]
+ args: List[Expr]
The arguments to the call.
Returns
-------
- call: BaseExpr
+ call: Expr
A call taking the variable as a function.
"""
# pylint: disable=import-outside-toplevel
- # TODO(@relax-team): replace with Relax base class after it's
introduced
- if all(isinstance(x, RelaxExpr) for x in args):
+ if all(isinstance(x, Number | PrimExpr) for x in args):
+ return tvm.tirx.call_tir(self, *args)
+
+ if all(isinstance(x, Expr) for x in args):
from tvm import relax
return relax.Call(self, args)
Review Comment:
In `GlobalVar.__call__`, the check `all(isinstance(x, Number | PrimExpr) for
x in args)` will incorrectly evaluate to `True` when `args` is empty (since
`all([])` is `True` in Python). This causes nullary Relax function calls (e.g.,
`gv()`) to be incorrectly dispatched to `tvm.tirx.call_tir` instead of
`relax.Call`.
Adding a check to ensure `args` is not empty before dispatching to
`call_tir` resolves this issue.
```suggestion
if args and all(isinstance(x, Number | PrimExpr) for x in args):
return tvm.tirx.call_tir(self, *args)
if all(isinstance(x, Expr) for x in args):
from tvm import relax
return relax.Call(self, args)
```
##########
python/tvm/relax/expr.py:
##########
@@ -40,11 +40,40 @@
# It is a workaround for mypy:
https://github.com/python/mypy/issues/7866#issuecomment-549454370
# This feature is not supported until python 3.10:
# https://docs.python.org/3.10/whatsnew/3.10.html#pep-613-typealias
-Expr = tvm.ir.RelaxExpr
+Expr = tvm.ir.Expr
Type = tvm.ir.Type # pylint: disable=invalid-name
GlobalVar = tvm.ir.GlobalVar
+def prim_value(value: PrimExpr | int | float, dtype: str | None = None) ->
PrimExpr:
+ """Convert a Python scalar or primitive expression to ``PrimExpr``.
+
+ Parameters
+ ----------
+ value : PrimExpr | int | float
+ The value to convert.
+
+ dtype : Optional[str]
+ The dtype to use when converting Python numeric values.
+
+ Returns
+ -------
+ result : PrimExpr
+ The converted primitive expression. Existing ``PrimExpr`` inputs are
+ returned unchanged.
+ """
+ if isinstance(value, PrimExpr):
+ return value
+ if isinstance(value, bool | int):
+ return tvm.tirx.IntImm(dtype or "int64", int(value))
Review Comment:
Since `bool` is a subclass of `int` in Python, `isinstance(value, bool |
int)` will match boolean values. If `dtype` is not specified, it will default
to `"int64"`, converting `True`/`False` to `IntImm("int64", 1/0)` instead of
`IntImm("bool", 1/0)`. This can cause type mismatches in operators expecting
boolean conditions (like `assert_op` or `if`).
Handling `bool` explicitly before `int` preserves the correct boolean type.
```suggestion
if isinstance(value, bool):
return tvm.tirx.IntImm(dtype or "bool", int(value))
if isinstance(value, int):
return tvm.tirx.IntImm(dtype or "int64", value)
```
##########
python/tvm/relax/frontend/onnx/onnx_frontend.py:
##########
@@ -478,15 +478,15 @@ def base_impl(cls, bb, inputs, attr, params):
x = _to_numpy(inputs[0])
y = _to_numpy(inputs[1])
output = cls.numpy_op(x, y) # pylint: disable=not-callable
- if isinstance(x, relax.PrimValue) and isinstance(y,
relax.PrimValue):
- return relax.PrimValue(output.item())
+ if isinstance(x, tvm.tirx.PrimExpr) and isinstance(y,
tvm.tirx.PrimExpr):
+ return relax.prim_value(output.item())
Review Comment:
The check `isinstance(x, tvm.tirx.PrimExpr)` will always evaluate to `False`
because `x` and `y` are Python scalars returned by `_to_numpy`. To correctly
check if the original inputs were `PrimExpr`s, we should check `inputs[0]` and
`inputs[1]` instead.
```suggestion
if isinstance(inputs[0], tvm.tirx.PrimExpr) and
isinstance(inputs[1], tvm.tirx.PrimExpr):
return relax.prim_value(output.item())
```
--
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]
