-
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
[BYOC][ACL] Fix list is not supported as an input node #10801
Merged
Merged
Changes from 11 commits
Commits
Show all changes
14 commits
Select commit
Hold shift + click to select a range
5845760
[BYOC][ACL] Fix list is not supported as an input node
937e91a
fix clang lint error
b47f26a
fix compile warnning
4089f17
fix python module import error
1d3aebc
rename concatenate test file
d89b7d4
fix always MakeACLTensor with same eid 0
8c87d45
do not offload concat default
6c03b19
fix concattnate test failure
9d36318
fix test failure
ca28694
fix lint error
9bb6421
fix lint
56eb714
remove global var offload_concat
b219357
support concatenate with pattern table mechanism
4603c12
disable pylint dangerous-default-value warning
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Rather than using a global var, can we make use of
arm_compute_lib_pattern_table
?tvm/python/tvm/relay/op/contrib/arm_compute_lib.py
Line 131 in 8a0472f
i.e. register
concat
pattern ifoffload_concat == True
.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.
Thanks for taking a look @masahi. Currently ACL has a mixture of registering operations using composite functions and
_register_external_op_helper
which adds thetarget.arm_compute_lib
attribute, which is not ideal. I think if we wanted to do this for concat it would need to be registered using pattern table rather than the other mechanism.In the future we should probably use the pattern table for all operations.
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.
Yeah, if we are adding a new op support, why not add it via pattern like
tvm/python/tvm/relay/op/contrib/arm_compute_lib.py
Line 134 in 8a0472f
Then it is easy to enable / disable a pattern based on some flags. @DzAvril
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.
Updated, please review. I'm not sure it is what you want because it seems a little weird since
arm_compute_lib_pattern_table
aims to register fusing patterns for ops and registering op attr is out of it.BTW, this commit triggered a
Sanity Check
error: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.
Actually I don't know what
@tvm.ir.register_op_attr("nn.conv2d", "target.arm_compute_lib")
thing is for... Why do you need both the pattern +check
function attvm/python/tvm/relay/op/contrib/arm_compute_lib.py
Line 134 in 8a0472f
tvm/python/tvm/relay/op/contrib/arm_compute_lib.py
Line 223 in 8a0472f
and
register_op_attr
thing attvm/python/tvm/relay/op/contrib/arm_compute_lib.py
Lines 291 to 292 in 8a0472f
?
I never needed the latter in the BYOCs I worked on.
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.
Gently ping @lhutton1, can you help to explain this to masai?
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.
@DzAvril, @lhutton1 said "I think if we wanted to do this for concat it would need to be registered using pattern table rather than the other mechanism". This is also what I'm suggesting above. Can you do that? You shouldn't need
@tvm.ir.register_op_attr
thing for concat. You just need to addis_op("concatenate")
etc + check function which you already have under@tvm.ir.register_op_attr(concatenate)
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.
Yeah I don't think we need to register conv2d like this either as it should have already been picked up by the pattern table. This way of registering operators predates the pattern table so I guess this was just overlooked. We can follow up in a separate PR to move all operators so that they are registered using the pattern table - which I believe is the way other BYOC's are going. Apologies for the confusion @DzAvril, like @masahi said, it would be best to register concatenate (and other operators added in the future) using the pattern table since this is the preferred mechanism.
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.
Sorry for missing some comments above. I have a question about the
concatenate_pattern
, see code below.As the input of
concatenate
is a tuple of tensors, what the pattern of the parameters should be like?wildcard()
in the code block is not working.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.
can you try
is_tuple(None)
? See #7754