-
Notifications
You must be signed in to change notification settings - Fork 75
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
Add ComputeEngine Property for choosing Engine #2849
base: main
Are you sure you want to change the base?
Conversation
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.
I'm glad to review this PR, initiating additional compute engine for QNN. 👍✨
This PR adds a new LayerComputeEngine
property in common.h
and includes it as a property. Here're some comments from my side.
ml::train::LayerComputeEngine | ||
getComputeEngine(const std::vector<std::string> &props) { |
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.
Function prototype for getComputeEngine
is missing in header. Isn't it required to be declared in header?
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 your comment. This getComputeEngine function will be removed in PR #2849.
It moved and renamed as "parseComputeEngine"
#endif | ||
} | ||
|
||
auto &ac = nntrainer::AppContext::Global(); |
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.
Do you think it holds for both ml::train::LayerComputeEngine::CPU
and ml::train::LayerComputeEngine::QNN
?
If not, you may need to add more condition to check it.
I think it would be better to add a case to check QNN
with NYI exception.
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.
No. it doesn't. This will changed in PR #2848 too.
nntrainer/nntrainer/layers/layer_node.cpp
Line 140 in dbbde82
auto &eg = nntrainer::Engine::Global(); |
#endif | ||
} | ||
|
||
auto &ac = nntrainer::AppContext::Global(); |
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.
ditto.
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.
ditto.
This PR add ComputeEngine Enum Property. Enum elements are "cpu", "gpu", "qnn" for now. The property format is "engine=qnn". **Self evaluation:** 1. Build test: [X]Passed [ ]Failed [ ]Skipped 2. Run test: [X]Passed [ ]Failed [ ]Skipped Signed-off-by: jijoong.moon <[email protected]>
869d348
to
5a11693
Compare
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.
LGTM
// const LayerComputeEngine &compute_engine = LayerComputeEngine::GPU) | ||
// { | ||
// return createLayer(LayerType::LAYER_RMSNORM, properties, compute_engine); | ||
// } |
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.
let's remove it
Add Compute Engine Property
This PR add ComputeEngine Enum Property.
Enum elements are "cpu", "gpu", "qnn" for now.
The property format is "engine=qnn".
Self evaluation:
Signed-off-by: jijoong.moon [email protected]