-
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][Hexagon] Implement Cast F32ToF16 and F16ToF32 Slice Op #11561
Conversation
cc @mehrdadh |
|
||
|
||
if __name__ == "__main__": | ||
sys.exit(pytest.main(sys.argv)) |
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.
don't need to send a commit for this, but if you have updated the PR please change this to tvm.testing.main()
cc @Lunderberg @cconvey @csullivan for review |
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.
One small change that should be made before merging, and a question on future use of get_layout_transform_fn
, but otherwise LGTM
@@ -49,4 +64,10 @@ def get_layout_transform_fn(layout): | |||
return n11c_1024c_2d | |||
if layout == "n11c-1024c-1d": | |||
return n11c_1024c_1d | |||
if layout == "nhwc-4h2w32c2w-2d": |
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.
In the future, is this function intended to remain a mapping from string to layout function, or will it have more dynamic behavior? If the former, then it may be useful to define a dictionary instead, since that would allow tests to be parametrized over each layout.
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.
Hi Eric @Lunderberg , Thank you for your time and helpful inputs. I think for now we are sticking to pre-defined layouts. But we don't want the tests to be parameterized over each layout since the tests wouldn't support each one of them.
|
||
|
||
if __name__ == "__main__": | ||
sys.exit(tvm.testing.main(sys.argv)) |
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.
tvm.testing.main
takes no arguments and internally calls sys.exit
, so this like should instead be tvm.testing.main()
@@ -194,4 +194,4 @@ def test_cast_fp32_fp16_slice( | |||
|
|||
|
|||
if __name__ == "__main__": | |||
sys.exit(tvm.testing.main(sys.argv)) | |||
sys.exit(tvm.testing.main()) |
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.
Small nitpick, the sys.exit
can be removed as well.
if __name__ == "__main__":
tvm.testing.main()
@tvm-bot rerun |
import tvm.topi.hexagon.slice_ops as sl | ||
from ..infrastructure import allocate_hexagon_array, transform_numpy | ||
|
||
# pylint: disable=invalid-name |
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 you don't need this here. Please remove it and fix if any lint issue exists
.
from tvm import tir | ||
from ..utils import get_layout_transform_fn | ||
|
||
# pylint: disable=invalid-name |
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.
same here.
Hi @mehrdadh @Lunderberg , could you please check if the changes are OK? Thank you. |
Hi @mehrdadh @Lunderberg could you please check if the changes are OK? Thank you. |
LGTM! I'll wait for @Lunderberg to approve and merge it. |
Many thanks @arangasa @mehrdadh @Lunderberg! |
Thanks for contributing to TVM! Please refer to guideline https://tvm.apache.org/docs/contribute/ for useful information and tips. After the pull request is submitted, please request code reviews from Reviewers by @ them in the pull request thread.
cc @mehrdadh