[BugFix][UMA] Fix order issue in uma_lower #12447
Conversation
MichaelJKlaiber
force-pushed
the
issue/uma-tflite
branch
from
f61e1c4 to
2d13b43
Compare
|
@mbaret wondering if you know the answer to this one (see attached PR)? |
|
@areusch, you are referring to the question from #12410, right? " Seems the bug is in the lowering step from relay_prim_func to tir_prim_func. When collecting the input arguments, the order gets messed up. By replacing the function _get_tensors here by everything works perfectly fine for all the test case we provided including the tflite import Does anyone know if it is sufficient to rely on the .inputs parameters from te_cached_func ? " |
|
@manupak, could you also review? There are some minor changes in aot.py, as well |
python/tvm/testing/aot.py
Outdated
| dtype = str(param.type_annotation.dtype) | ||
| in_min, in_max = (np.iinfo(dtype).min, np.iinfo(dtype).max) | ||
| data = np.random.randint(in_min, high=in_max, size=data_shape, dtype=dtype) | ||
| if dtype == "float32": |
There was a problem hiding this comment.
I think we can use np.finfo to make it "generally" working for float dtypes
There was a problem hiding this comment.
Done ...
what do you mean by "we'll probably need a commit message prior to merging"?
There was a problem hiding this comment.
A general title and a short description. There is a ongoing proposal to make this formal -- apache/tvm-rfcs#88.
For this one, if you can propose and change the title and decription in the PR, Im happy to use that as the commit title and description to align to support the above ongoing effort.
Additionally, the fact that this fixes #12410 should be part of the description.
There was a problem hiding this comment.
Sorry if I was not clear before...
I was suggesting something like :
if np.issubdtype(dtype, np.floating) :
so this would generally work.
MichaelJKlaiber
force-pushed
the
issue/uma-tflite
branch
from
2c6da5a to
b54889d
Compare
MichaelJKlaiber
changed the title
manupak
left a comment
manupak
left a comment
There was a problem hiding this comment.
Thanks for the commit message!
Only a single comment and I think this is good to go...
python/tvm/testing/aot.py
Outdated
| dtype = str(param.type_annotation.dtype) | ||
| in_min, in_max = (np.iinfo(dtype).min, np.iinfo(dtype).max) | ||
| data = np.random.randint(in_min, high=in_max, size=data_shape, dtype=dtype) | ||
| if dtype == "float32": |
There was a problem hiding this comment.
Sorry if I was not clear before...
I was suggesting something like :
if np.issubdtype(dtype, np.floating) :
so this would generally work.
…le_and_inputs_from_tflite_file
…le_and_inputs_from_tflite_file
…_and_inputs_from_tflite_file
|
Thanks @MichaelJKlaiber! |
There was a flaw in uma_lower (see issue apache#12410) that lead in some case to a different argument ordering of the cached_func and the Relay function. This results in an incorrect lowering of the primfunc and eventually a wrong result of a run-time error, in some cases. This commit adds code to correct the described misbehavior and a unit test case to check this end-to-end functionality with a TFLITE model.
3 participants
There was a flaw in uma_lower (see issue #12410) that lead in some case to a different argument ordering of the cached_func and the Relay function. This results in an incorrect lowering of the primfunc and eventually a wrong result of a run-time error, in some cases.
This commit adds code to correct the described misbehavior and a unit test case to check this end-to-end functionality with a TFLITE model.