[Hexagon] Add support for linked-in model parameters #8865
Conversation
kparzysz-quic
requested review from
junrushao,
masahi,
tqchen,
vinx13 and
ZihengJiang
as code owners
|
cc @manupa-arm @leandron is this using same machinery you guys are using? would you mind reviewing? cc @areusch |
|
Gentle ping... |
| linked_params = | ||
| Downcast<Map<String, LinkedParam>>(attrs_dict[::tvm::tir::attr::kLinkedParams]); | ||
| found_linked_params = true; | ||
| continue; |
There was a problem hiding this comment.
nit : this seems searching for whether at least one function have linked_params, if so I think we dont need to continue searching. A further suggestion is to break it out to a function for that check w/o needing to maintain found_linked_params bool.
There was a problem hiding this comment.
This is traversing all functions and
- if
fis a "linked-params" function, we set it aside, - otherwise we add
fto the list of functions for codegen.
We need to skip the "linked-params" function, because the codegen path for it is different. We create the bool variable so that we know to generate the linked parameters function later on, but the variable is not the only effect of the loop, so there isn't much to gain by extracting it into a separate function.
There was a problem hiding this comment.
Ack, so this is just to skip the linked-params function -- might be better to add a comment.
I guess my original concern is it would be bit more readable if it was a different function to obtain the linked-params altogether rather than fusing it with the loop as you correctly note here -- it being not the only effect, WDYT?
cc: @d-smirnov , another occurence where linked-param will be used.
There was a problem hiding this comment.
We will still need to recognize it when adding functions to the codegen module (to avoid adding it).
There was a problem hiding this comment.
Ack and thanks for the explaination -- maybe a comment would help then :)
| Map<String, ObjectRef> attrs_dict = Downcast<Map<String, ObjectRef>>(kv.second->attrs->dict); | ||
| CHECK(attrs_dict.find(::tvm::tir::attr::kLinkedParams) != attrs_dict.end()) | ||
| << "no " << ::tvm::tir::attr::kLinkedParams << " attribute found!"; | ||
| linked_params = |
There was a problem hiding this comment.
Why are we replacing the map here (as opposed to setting it) ?
There was a problem hiding this comment.
What do you mean by "replacing" the map? Map is an ObjectRef, so we're not really copying anything here.
There was a problem hiding this comment.
I thought the reason for declaring the Map outside of the scope of loop is to 'Set' it -- hence the question.
Another way to ask the same thing : any reason to declare the Map outside of the for loop ?
There was a problem hiding this comment.
It's used after the loop. The map holds the parameters that we pass to LinkParameters.
There was a problem hiding this comment.
Hmmm, it seems this work because it is only set once in the loop.
Any reason not to codegen it here (rather than doing it down there) ?
There was a problem hiding this comment.
I'm going to add a check there to make sure it's not set more than once.
|
I'll add a testcase for Hexagon, but it won't run in the CI, because Hexagon is not built (it requires SDK). |
kparzysz-quic
requested review from
areusch,
comaniac,
jroesch,
merrymercy and
yzhliu
as code owners
Edit: make bulleted list. |
There was a problem hiding this comment.
sorry for the delay @kparzysz-quic . i think we can proceed with this since it uses the same mechanism we currently use in GraphRuntime C land. for AOT we'll need to likely go a different route, but we'll need to handle that in the C source codegen anyhow I believe.
| @@ -107,7 +109,71 @@ def test_alloc_vtcm(): | |||
| assert "HexagonBackendFreeVTCM" in calls | |||
|
|
|||
|
|
|||
| def test_linked_params_codegen(): | |||
| if not check_prereq_and_setup(): | |||
There was a problem hiding this comment.
(also ok for a follow-up) do you mind migrating this to the pytest decorator style and modifying the __main__ invocation:
if __name__ == "__main__":
sys.exit(pytest.main([__file__] + sys.argv[1:]))
There was a problem hiding this comment.
Sure, I'll do it, but I'd prefer to do it in a separate PR, since I'd need to change some extra files.
* [Hexagon] Add support for linked-in model parameters * Remove entry_func, since it's not used anywhere * Simplify linked-param codegen preparation a bit * Detect multiple linked-params functions * Add testcase to check for linked-param codegen * Empty commit to restart build
* [Hexagon] Add support for linked-in model parameters * Remove entry_func, since it's not used anywhere * Simplify linked-param codegen preparation a bit * Detect multiple linked-params functions * Add testcase to check for linked-param codegen * Empty commit to restart build
No description provided.