-
Notifications
You must be signed in to change notification settings - Fork 3.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[TOPI][ONNX] Fix for trilu and set_matrix_diag ops #11761
base: main
Are you sure you want to change the base?
Conversation
@mikepapadim can you make sure the tests pass? |
I am trying to reproduce them locally. Will update when I have them fixed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM. Do we have an update on the PR? I am trying to add triu
for our pytorch frontend and this PR could be really useful if merged! Thanks
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's probably a spurious import (e.g. typed Expr and IDE autoimported this when it should be tvms)
@mikepapadim I took a look and looks like we need to add
to here: tvm/python/tvm/topi/transform.py Line 922 in 057dd7c |
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from tables import Expr |
This PR addresses and extends issues of PRs #10873 and #9329.
Refactor the implementation of
set_matrix_diag
to enable support of thetrilu
operator in ONNX.@sfvaroglu @bfgoldstein @AndrewZhaoLuo