Skip to content
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

[HEXAGON] Slice ops added - add, subtract, multiply #11529

Merged
merged 12 commits into from
Jun 21, 2022

Conversation

trahman-quic
Copy link
Contributor

@trahman-quic trahman-quic commented Jun 1, 2022

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

@github-actions github-actions bot requested a review from mehrdadh June 1, 2022 22:40
@mehrdadh
Copy link
Member

mehrdadh commented Jun 2, 2022

@trahman-quic please fix the lint issue.

@trahman-quic trahman-quic force-pushed the trahman-quic/sliced-add-sub-mul branch from 6fb6b0a to f90a664 Compare June 2, 2022 19:23
@trahman-quic trahman-quic changed the title [UPSTREAM][HEXAGON] Slice ops added - add, subtract, multiply [HEXAGON] Slice ops added - add, subtract, multiply Jun 2, 2022
@trahman-quic trahman-quic force-pushed the trahman-quic/sliced-add-sub-mul branch from f90a664 to 009e536 Compare June 2, 2022 19:35
@trahman-quic
Copy link
Contributor Author

Hi @mehrdadh, looks like my PR passed all the checks. Is it good to merge now?

@mehrdadh
Copy link
Member

mehrdadh commented Jun 7, 2022

let's wait for this(#11613) and rerun this PR

Copy link
Member

@mehrdadh mehrdadh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good. I added few comments

tests/python/contrib/test_hexagon/infrastructure.py Outdated Show resolved Hide resolved
@@ -0,0 +1,230 @@
# Licensed to the Apache Software Foundation (ASF) under one
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please move this file to tests/python/contrib/test_hexagon/topi/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

python/tvm/topi/hexagon/slice_ops/__init__.py Outdated Show resolved Hide resolved
python/tvm/topi/hexagon/slice_ops/add_subtract_multiply.py Outdated Show resolved Hide resolved
python/tvm/topi/hexagon/utils.py Outdated Show resolved Hide resolved


def n11c_1024c_1d(n, h, w, c):
"""Return index map for n11c_1024 1d layout"""
Copy link
Contributor Author

@trahman-quic trahman-quic Jun 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mehrdadh I am having the same lint error about argument variable "w" here as well.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since these are utility functions, it would be great if you can use better naming.

output_transformed_layout = get_layout_transform_fn(output_layout)
s.transform_layout(block, buffer=("write", 0), index_map=output_transformed_layout)

n, h, w, c = s.get_loops(block)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mehrdadh lint error is complaining about the variable name "w". It's not complaining about "n" or "h" or "c". Is it a reasonable error? Is there a way to stick to these simple(single letter) variable names?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you're right. we might need to add pylint disable in this file.

@trahman-quic trahman-quic force-pushed the trahman-quic/sliced-add-sub-mul branch from db9be6b to 1744403 Compare June 10, 2022 16:29
@trahman-quic trahman-quic force-pushed the trahman-quic/sliced-add-sub-mul branch from 1744403 to 188fe44 Compare June 10, 2022 18:26
@trahman-quic trahman-quic force-pushed the trahman-quic/sliced-add-sub-mul branch from 87b5dc4 to 1a9e0eb Compare June 15, 2022 20:40
@trahman-quic
Copy link
Contributor Author

Hi @mehrdadh, I merged main branch into my branch and the hexagon tests are passing now. But now it's failing on a QEMU test. Could you please give me some ideas on what might be causing this?

@mehrdadh
Copy link
Member

@trahman-quic I think this was resolved. Could you rebase and push to run the job?

@trahman-quic trahman-quic force-pushed the trahman-quic/sliced-add-sub-mul branch from 6bacd64 to 6bc4d4a Compare June 20, 2022 19:27
@trahman-quic
Copy link
Contributor Author

Hi @mehrdadh, all the tests are passing now. Pinging you to see if it is good to merge now.

Copy link
Member

@mehrdadh mehrdadh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@mehrdadh mehrdadh merged commit bc75487 into apache:main Jun 21, 2022
@mehrdadh
Copy link
Member

@trahman-quic PR is merged now, thanks!

blackkker pushed a commit to blackkker/tvm that referenced this pull request Jul 7, 2022
* [UPSTREAM][HEXAGON] Slice ops added - add, subtract, multiply

* Change to v68

* Change transform_numpy function call

* Do not disbale pylint errors and fix them

* Fix variable names

* Move the test file to topi

* Resolve conflict

* Modify init
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants