Conversation
comaniac
left a comment
There was a problem hiding this comment.
Sorry for the late review. In general as I mentioned in the RFC, this PR is too large for people to review and I believe this is the major reason why it's hard to collect feedbacks. I just reviewed the high-level Python parts and will try to review the rest parts when I got time. Meanwhile, It would be better to remove most non-infra parts such as operators first and send follow-up PRs.
In addition, I'm still not very comfortable with the way of verifying the generated C++ code using text comparison. After the NNAPI compiler is available in the CI, we could just verify if the generated code can be successfully compiled, but we probably cannot verify its run time functionality as other BYOC backends.
cc @zhiics
| Note | ||
| ---- |
There was a problem hiding this comment.
| Note | |
| ---- | |
| Notes | |
| ----- |
| from .converter import Converter | ||
|
|
||
|
|
||
| def convert_relayir_to_nnapi(func): |
There was a problem hiding this comment.
I won't call this "convert". Following the naming convention of existing BYOC backends, this is the codegen/compiler of NNAPI, so I'll probably just call it nnapi_compiler. Also, can you register it like the following?
@tvm._ffi.register_func("relay.ext.nnapicompiler")
def nnapi_compiler(func):Meanwhile, the directory name relayir_to_nnapi_converter should also be improved. Maybe just name it compiler or codegen.
There was a problem hiding this comment.
Hi, I plan to name it "codegen" if that's OK. (I don't want to collide with the Python built-in function "compile")
As an extra explanation that matters, the external compiler is actually a C++ stub in src/relay/backend/contrib/android_nnapi/codegen.cc, and it's that thing that gets registered as relay.ext.android_nnapi.
The existence of the C++ stub is a historical issue, however, it's not a complex file after all, so would you agree if I completely remove the C++ stub and make all of relay.ext.android_nnapi pythonic?
There was a problem hiding this comment.
We actually prefer a reverse way that makes most of the implementations in C++ instead of Python for better performance. However, given that would be a huge effort, it should be fine for this specific backend to be all in Python IMHO.
| ------- | ||
| dtype: str | ||
| dtype of the queried operand | ||
|
|
There was a problem hiding this comment.
remove empty line. (ditto to other functions in all Python files).
| """Converts a Relay IR Function into Android NNAPI C++ class | ||
|
|
||
| Parameters | ||
| ---------------------- |
There was a problem hiding this comment.
| ---------------------- | |
| ---------- |
|
|
||
| """ | ||
|
|
||
| DEFAULT_OPTIONS = { |
There was a problem hiding this comment.
Add more comments to explain each fields, such as name and api_level.
| if value_dict["type"] == "memory_ptr": | ||
| return value_dict["value"] | ||
| assert False, "Unreachable" |
There was a problem hiding this comment.
| if value_dict["type"] == "memory_ptr": | |
| return value_dict["value"] | |
| assert False, "Unreachable" | |
| assert value_dict["type"] == "memory_ptr": | |
| return value_dict["value"] |
So that you don't need to disable inconsistent-return-statements.
|
|
||
| Returns | ||
| ------- | ||
| obj: |
| return None | ||
| return self._export_obj["constants"][value_dict["value"]] | ||
|
|
||
| def is_FuseCode(self, idx): # pylint: disable=invalid-name |
There was a problem hiding this comment.
| def is_FuseCode(self, idx): # pylint: disable=invalid-name | |
| def is_fuse_code(self, idx): |
| if value not in { | ||
| "ANEURALNETWORKS_FUSED_NONE", | ||
| "ANEURALNETWORKS_FUSED_RELU", | ||
| "ANEURALNETWORKS_FUSED_RELU1", | ||
| "ANEURALNETWORKS_FUSED_RELU6", | ||
| }: | ||
| return False | ||
| return True |
There was a problem hiding this comment.
| if value not in { | |
| "ANEURALNETWORKS_FUSED_NONE", | |
| "ANEURALNETWORKS_FUSED_RELU", | |
| "ANEURALNETWORKS_FUSED_RELU1", | |
| "ANEURALNETWORKS_FUSED_RELU6", | |
| }: | |
| return False | |
| return True | |
| return value in { | |
| "ANEURALNETWORKS_FUSED_NONE", | |
| "ANEURALNETWORKS_FUSED_RELU", | |
| "ANEURALNETWORKS_FUSED_RELU1", | |
| "ANEURALNETWORKS_FUSED_RELU6", | |
| } |
|
|
||
| def convert(self, func): | ||
| """Converts a Relay IR Function into Android NNAPI C++ class source code | ||
| Parameters |
|
Hey Cody, thank you for taking time to review this. I really appreciate this. For the suggestion of removing operator implementations in this PR, I would agree that this would make the PR smaller, but please do take note that the main body of this PR is already infrastructure, so removing operators would not help much. The tests can be made to test for compilability only instead of text comparing, and I'll do this on the next commit to this branch. Apart from these, it looks like there's a few coding style/document suggestions, so I'll keep working on the refactor. |
This commit implements the basic structure of Android NNAPI codegen with the BYOC mechanism: * Basic graph partition using pattern-based rules * RPC-based graph partition * relay.ext.android_nnapi BYOC codegen
s.a. TVM PR #8076
for operator testing of Android NNAPI BYOC s.a. PR #8076
all of Android NNAPI BYOC Pythonic s.a. PR #8076
to "Compiler" s.a. PR #8076
costs between TVM and Android NNAPI in the RPC
partitioner
s.a. PR #8076
|
what's the status of this PR? |
|
Thanks for reminding. I'm closing this PR for now as @melsonlai is no longer working on this RFC. The guy that takes over his work will file a new one later. |
|
May I ask how this PR is going and with what it has been replaced, @comaniac ? Is there still interested in implenting and NnApi backend for TVM? If so, what would the next steps have to be for it? |
|
Unfortunately the group of authors worked on this PR was no longer contributing due to their own reason, so AFAIK there's no plan for NNAPI upstreaming ATM. You are welcome to propose one. |
This commit implements the basic structure of Android NNAPI codegen with the BYOC mechanism:
[RFC][BYOC] Android NNAPI Integration
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.