You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tvm.apache.org by GitBox <gi...@apache.org> on 2022/06/17 15:08:14 UTC
[GitHub] [tvm] mikepapadim opened a new pull request, #11761: [TOPI][ONNX] Fix for trilu and set_matrix_diag ops
mikepapadim opened a new pull request, #11761:
URL: https://github.com/apache/tvm/pull/11761
This PR addresses and extends issues of PRs #10873 and #9329.
Refactor the implementation of `set_matrix_diag` to enable support of the `trilu` operator in ONNX.
@sfvaroglu @bfgoldstein @AndrewZhaoLuo
--
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
[GitHub] [tvm] shingjan commented on a diff in pull request #11761: [TOPI][ONNX] Fix for trilu and set_matrix_diag ops
Posted by GitBox <gi...@apache.org>.
shingjan commented on code in PR #11761:
URL: https://github.com/apache/tvm/pull/11761#discussion_r909122902
##########
python/tvm/topi/transform.py:
##########
@@ -17,9 +17,12 @@
# pylint: disable=invalid-name,consider-using-enumerate,redefined-outer-name
"""Injective transformation operators"""
from __future__ import absolute_import as _abs
+import numpy as np
+from tables import Expr
Review Comment:
I am hitting the following error:
```
E 0: tvm::codegen::CodeGenLLVM::CreateBufferPtr(llvm::Value*, tvm::runtime::DataType, llvm::ArrayRef<llvm::Value*>, tvm::runtime::DataType)
E at /home/yj/tvm/src/target/llvm/codegen_llvm.cc:737
E File "/home/yj/tvm/src/target/llvm/codegen_llvm.cc", line 737
E TVMError:
E ---------------------------------------------------------------
E An error occurred during the execution of TVM.
E For more information, please see: https://tvm.apache.org/docs/errors.html
E ---------------------------------------------------------------
E Check failed: (index->getType()->isIntegerTy()) is false: Expected buffer index to be an integer
```
I wonder if there are still places in the codegen that assumes `k1,k2` as integer
--
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
[GitHub] [tvm] AndrewZhaoLuo commented on a diff in pull request #11761: [TOPI][ONNX] Fix for trilu and set_matrix_diag ops
Posted by GitBox <gi...@apache.org>.
AndrewZhaoLuo commented on code in PR #11761:
URL: https://github.com/apache/tvm/pull/11761#discussion_r910444115
##########
python/tvm/topi/transform.py:
##########
@@ -17,9 +17,12 @@
# pylint: disable=invalid-name,consider-using-enumerate,redefined-outer-name
"""Injective transformation operators"""
from __future__ import absolute_import as _abs
+import numpy as np
+from tables import Expr
Review Comment:
I think it's probably a spurious import (e.g. typed Expr and IDE autoimported this when it should be tvms)
--
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
[GitHub] [tvm] shingjan commented on a diff in pull request #11761: [TOPI][ONNX] Fix for trilu and set_matrix_diag ops
Posted by GitBox <gi...@apache.org>.
shingjan commented on code in PR #11761:
URL: https://github.com/apache/tvm/pull/11761#discussion_r912219906
##########
python/tvm/topi/transform.py:
##########
@@ -17,9 +17,12 @@
# pylint: disable=invalid-name,consider-using-enumerate,redefined-outer-name
"""Injective transformation operators"""
from __future__ import absolute_import as _abs
+import numpy as np
+from tables import Expr
Review Comment:
```suggestion
```
--
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
[GitHub] [tvm] shingjan commented on a diff in pull request #11761: [TOPI][ONNX] Fix for trilu and set_matrix_diag ops
Posted by GitBox <gi...@apache.org>.
shingjan commented on code in PR #11761:
URL: https://github.com/apache/tvm/pull/11761#discussion_r912219771
##########
python/tvm/topi/transform.py:
##########
@@ -17,9 +17,12 @@
# pylint: disable=invalid-name,consider-using-enumerate,redefined-outer-name
"""Injective transformation operators"""
from __future__ import absolute_import as _abs
+import numpy as np
Review Comment:
```suggestion
```
--
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
[GitHub] [tvm] AndrewZhaoLuo commented on a diff in pull request #11761: [TOPI][ONNX] Fix for trilu and set_matrix_diag ops
Posted by GitBox <gi...@apache.org>.
AndrewZhaoLuo commented on code in PR #11761:
URL: https://github.com/apache/tvm/pull/11761#discussion_r910444115
##########
python/tvm/topi/transform.py:
##########
@@ -17,9 +17,12 @@
# pylint: disable=invalid-name,consider-using-enumerate,redefined-outer-name
"""Injective transformation operators"""
from __future__ import absolute_import as _abs
+import numpy as np
+from tables import Expr
Review Comment:
I think it's probably a spurious import (e.g. typed Expr and his IDE autoimported this)
--
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
[GitHub] [tvm] shingjan commented on a diff in pull request #11761: [TOPI][ONNX] Fix for trilu and set_matrix_diag ops
Posted by GitBox <gi...@apache.org>.
shingjan commented on code in PR #11761:
URL: https://github.com/apache/tvm/pull/11761#discussion_r909116854
##########
python/tvm/topi/transform.py:
##########
@@ -17,9 +17,12 @@
# pylint: disable=invalid-name,consider-using-enumerate,redefined-outer-name
"""Injective transformation operators"""
from __future__ import absolute_import as _abs
+import numpy as np
+from tables import Expr
Review Comment:
are these two lines necessary? i am getting a `tables` not found on my end and it seems like those two libs aren't referenced anyway
--
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
[GitHub] [tvm] shingjan commented on a diff in pull request #11761: [TOPI][ONNX] Fix for trilu and set_matrix_diag ops
Posted by GitBox <gi...@apache.org>.
shingjan commented on code in PR #11761:
URL: https://github.com/apache/tvm/pull/11761#discussion_r909122902
##########
python/tvm/topi/transform.py:
##########
@@ -17,9 +17,12 @@
# pylint: disable=invalid-name,consider-using-enumerate,redefined-outer-name
"""Injective transformation operators"""
from __future__ import absolute_import as _abs
+import numpy as np
+from tables import Expr
Review Comment:
I am hitting the following error:
```
E 0: tvm::codegen::CodeGenLLVM::CreateBufferPtr(llvm::Value*, tvm::runtime::DataType, llvm::ArrayRef<llvm::Value*>, tvm::runtime::DataType)
E at /home/yj/tvm/src/target/llvm/codegen_llvm.cc:737
E File "/home/yj/tvm/src/target/llvm/codegen_llvm.cc", line 737
E TVMError:
E ---------------------------------------------------------------
E An error occurred during the execution of TVM.
E For more information, please see: https://tvm.apache.org/docs/errors.html
E ---------------------------------------------------------------
E Check failed: (index->getType()->isIntegerTy()) is false: Expected buffer index to be an integer
```
--
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
[GitHub] [tvm] shingjan commented on pull request #11761: [TOPI][ONNX] Fix for trilu and set_matrix_diag ops
Posted by GitBox <gi...@apache.org>.
shingjan commented on PR #11761:
URL: https://github.com/apache/tvm/pull/11761#issuecomment-1172667567
@mikepapadim I took a look and looks like we need to add
```
if not isinstance(k_one, tvm.te.Tensor):
k_one = const_vector(np.asarray([k_one], dtype=np.int64))
if not isinstance(k_two, tvm.te.Tensor):
k_two = const(np.asarray([k_two], dtype=np.int64))
```
to here:https://github.com/apache/tvm/blob/057dd7caf0011bf99faefc7d8c1e472b6114fc1d/python/tvm/topi/transform.py#L922
--
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
[GitHub] [tvm] AndrewZhaoLuo commented on pull request #11761: [TOPI][ONNX] Fix for trilu and set_matrix_diag ops
Posted by GitBox <gi...@apache.org>.
AndrewZhaoLuo commented on PR #11761:
URL: https://github.com/apache/tvm/pull/11761#issuecomment-1162028505
@mikepapadim can you make sure the tests pass?
--
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
[GitHub] [tvm] mikepapadim commented on pull request #11761: [TOPI][ONNX] Fix for trilu and set_matrix_diag ops
Posted by GitBox <gi...@apache.org>.
mikepapadim commented on PR #11761:
URL: https://github.com/apache/tvm/pull/11761#issuecomment-1162991795
I am trying to reproduce them locally. Will update when I have them fixed.
--
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