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.

Sign up for GitHub

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

Jump to bottom

[Target][Lowering] Update Op Intrinsic Lowering Mechanism And Intrinsic Lowering Pass #7809

Merged
merged 41 commits into from
Apr 24, 2021
Merged

[Target][Lowering] Update Op Intrinsic Lowering Mechanism And Intrinsic Lowering Pass #7809

merged 41 commits into from
Apr 24, 2021

Conversation

zxybazh
Copy link
Member

@zxybazh zxybazh commented Apr 8, 2021 •
edited
Loading

This PR updated the intrinsic lowering pass to support the new op registry and avoid overloading the global tvm registry. Meanwhile, it kept the fallback mechanism to find the most suitable lower intrinsic function, e.g., llvm.FLowerIntrinsic vs. default.FLowerIntrinsic. All previous op registration are ported to new functions, and some missing ops would be added in separate PR.

@zxybazh
Copy link
Member Author

zxybazh commented Apr 12, 2021

Python side op lowering function has been changed from register_func to register_op.

@junrushao junrushao self-assigned this Apr 13, 2021
@junrushao
Copy link
Member

junrushao commented Apr 13, 2021 •
edited
Loading

Thanks for the contribution! A few high-level comments:

  1. Do not add any C API - we can use packed func to expose the interface. Op is a compile-time concept so please move the registration logic out of the runtime :-)
  2. Please change register_op to some name like register_intrin_lowering, and consider accepting a target kind like default/llvm/cuda

@zxybazh
Copy link
Member Author

zxybazh commented Apr 13, 2021

Thanks for the advice! I have changed the name of function in python-side to register_op_intrin_lowering and removed change to C API. Instead, I utilized the global function registration and packedfunc to register op intrinsic lowering function.

@zxybazh zxybazh changed the title [WIP][Target][Lowering] Update Op Intrinsic Lowering Mechanism And Intrinsic Lowering Pass [Target][Lowering] Update Op Intrinsic Lowering Mechanism And Intrinsic Lowering Pass Apr 13, 2021
@zxybazh
Copy link
Member Author

zxybazh commented Apr 15, 2021

@junrushao1994 Please review, thanks!

junrushao
junrushao reviewed Apr 16, 2021
View reviewed changes
Copy link
Member

@junrushao junrushao left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution!

include/tvm/tir/op_attr_types.h Show resolved Hide resolved
include/tvm/tir/op_attr_types.h Outdated Show resolved Hide resolved
include/tvm/tir/op_attr_types.h Outdated Show resolved Hide resolved
junrushao
junrushao requested changes Apr 16, 2021
View reviewed changes
Copy link
Member

@junrushao junrushao left a comment

Choose a reason for hiding this comment

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

Please find some further comments :-)

python/tvm/ir/op.py Outdated Show resolved Hide resolved
src/ir/op.cc Outdated Show resolved Hide resolved
src/ir/op.cc Show resolved Hide resolved
python/tvm/ir/op.py Outdated Show resolved Hide resolved
@tqchen
Copy link
Member

tqchen commented Apr 20, 2021

note, please do not checkin vta-hw submodule change(do a git submodule update)

@zxybazh zxybazh force-pushed the lower branch 5 times, most recently from 5521ac6 to a04f2c2 Compare April 20, 2021 17:19
junrushao
junrushao approved these changes Apr 21, 2021
View reviewed changes
Copy link
Member

@junrushao junrushao left a comment

Choose a reason for hiding this comment

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

I don't have other comments :-) Thanks for the contribution!

@junrushao
Copy link
Member

CC: @comaniac @yzhliu @icemelon9

@junrushao
Copy link
Member

@tqchen would you like to take a second look? If there is other comments, I will get it merged the other day

tqchen
tqchen requested changes Apr 22, 2021
View reviewed changes
Copy link
Member

@tqchen tqchen left a comment

Choose a reason for hiding this comment

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

two minor nits mainly on the api and use TypedPackedFunc over PackedFunc. Overall LGTM!

include/tvm/ir/op.h Outdated Show resolved Hide resolved
src/target/llvm/intrin_rule_hexagon.cc Outdated Show resolved Hide resolved
src/target/llvm/intrin_rule_llvm.cc Outdated Show resolved Hide resolved
@junrushao junrushao requested a review from tqchen April 23, 2021 03:54
tqchen
tqchen requested changes Apr 23, 2021
View reviewed changes
Copy link
Member

@tqchen tqchen left a comment

Choose a reason for hiding this comment

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

Thanks for keep improving the PR! Taking a close look this time. I think we can merge once the current set of comments are addressed

src/target/llvm/intrin_rule_llvm.cc Outdated Show resolved Hide resolved
src/target/llvm/intrin_rule_llvm.cc Outdated Show resolved Hide resolved
src/target/llvm/intrin_rule_llvm.cc Outdated Show resolved Hide resolved
src/target/llvm/intrin_rule_llvm.cc Outdated Show resolved Hide resolved
src/target/llvm/intrin_rule_llvm.cc Outdated Show resolved Hide resolved
src/tir/transforms/lower_intrin.cc Outdated Show resolved Hide resolved
tqchen
tqchen approved these changes Apr 23, 2021
View reviewed changes
Copy link
Member

@tqchen tqchen left a comment

Choose a reason for hiding this comment

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

LTGM! Thanks @zxybazh !

@junrushao junrushao merged commit 251f571 into apache:main Apr 24, 2021
@junrushao
Copy link
Member

Thanks @zxybazh @tqchen!

@zxybazh zxybazh deleted the lower branch April 24, 2021 18:13
echuraev pushed a commit to echuraev/tvm that referenced this pull request Apr 29, 2021
@zxybazh @echuraev
[Target][Lowering] Update Op Intrinsic Lowering Mechanism And Intrins…
4153ad5 
…ic Lowering Pass (apache#7809)

This PR updated the intrinsic lowering pass to support the new op registry and avoid overloading the global tvm registry. Meanwhile, it kept the fallback mechanism to find the most suitable lower intrinsic function, e.g., llvm.FLowerIntrinsic vs. default.FLowerIntrinsic. All previous op registration are ported to new functions, and some missing ops would be added in separate PR.
umangyadav pushed a commit to umangyadav/tvm that referenced this pull request May 5, 2021
@zxybazh @umangyadav
[Target][Lowering] Update Op Intrinsic Lowering Mechanism And Intrins…
0282d69 
…ic Lowering Pass (apache#7809)

This PR updated the intrinsic lowering pass to support the new op registry and avoid overloading the global tvm registry. Meanwhile, it kept the fallback mechanism to find the most suitable lower intrinsic function, e.g., llvm.FLowerIntrinsic vs. default.FLowerIntrinsic. All previous op registration are ported to new functions, and some missing ops would be added in separate PR.
trevor-m pushed a commit to trevor-m/tvm that referenced this pull request May 6, 2021
@zxybazh
[Target][Lowering] Update Op Intrinsic Lowering Mechanism And Intrins…
657bbaa 
…ic Lowering Pass (apache#7809)

This PR updated the intrinsic lowering pass to support the new op registry and avoid overloading the global tvm registry. Meanwhile, it kept the fallback mechanism to find the most suitable lower intrinsic function, e.g., llvm.FLowerIntrinsic vs. default.FLowerIntrinsic. All previous op registration are ported to new functions, and some missing ops would be added in separate PR.
trevor-m pushed a commit to trevor-m/tvm that referenced this pull request May 6, 2021
@zxybazh
[Target][Lowering] Update Op Intrinsic Lowering Mechanism And Intrins…
9e450b4 
…ic Lowering Pass (apache#7809)

This PR updated the intrinsic lowering pass to support the new op registry and avoid overloading the global tvm registry. Meanwhile, it kept the fallback mechanism to find the most suitable lower intrinsic function, e.g., llvm.FLowerIntrinsic vs. default.FLowerIntrinsic. All previous op registration are ported to new functions, and some missing ops would be added in separate PR.
trevor-m pushed a commit to trevor-m/tvm that referenced this pull request May 6, 2021
@zxybazh
[Target][Lowering] Update Op Intrinsic Lowering Mechanism And Intrins…
a36234e 
…ic Lowering Pass (apache#7809)

This PR updated the intrinsic lowering pass to support the new op registry and avoid overloading the global tvm registry. Meanwhile, it kept the fallback mechanism to find the most suitable lower intrinsic function, e.g., llvm.FLowerIntrinsic vs. default.FLowerIntrinsic. All previous op registration are ported to new functions, and some missing ops would be added in separate PR.
trevor-m pushed a commit to neo-ai/tvm that referenced this pull request May 11, 2021
@zxybazh @trevor-m
[Target][Lowering] Update Op Intrinsic Lowering Mechanism And Intrins…
95f8a8c 
…ic Lowering Pass (apache#7809)

This PR updated the intrinsic lowering pass to support the new op registry and avoid overloading the global tvm registry. Meanwhile, it kept the fallback mechanism to find the most suitable lower intrinsic function, e.g., llvm.FLowerIntrinsic vs. default.FLowerIntrinsic. All previous op registration are ported to new functions, and some missing ops would be added in separate PR.
@junrushao junrushao mentioned this pull request Nov 1, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tqchen tqchen tqchen approved these changes

@junrushao junrushao junrushao approved these changes

Assignees

@junrushao junrushao

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@zxybazh @junrushao @tqchen