-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
[NPU] Use different graph init #26903
Conversation
00a8dd4
to
2cd5db5
Compare
8b87624
to
cfc0a15
Compare
cfc0a15
to
dd84811
Compare
dd84811
to
2a21c93
Compare
2a21c93
to
0cd7c33
Compare
build_jenkins |
@@ -132,14 +132,14 @@ class LevelZeroCompilerInDriver final : public ICompiler { | |||
void getNativeBinary(ze_graph_dditable_ext_curr_t& graphDdiTableExt, | |||
ze_graph_handle_t graphHandle, | |||
std::vector<uint8_t>& blob, | |||
uint8_t*& blobPtr, | |||
const uint8_t*& blobPtr, |
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.
Interesting combination.
If I read it correctly, this semantic implies that we can change the pointer itself, but want to keep the data, pointer is pointing to, constant. Is it really required for this case?
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 changed this because it was also changed in the driver. And pfnGetNativeBinary2 requires a const pointer. Also, I think it make more sense these changes. We need to keep data(blob) as constant, indeed.
typedef ze_result_t (ZE_APICALL *ze_pfnGraphGetNativeBinary_ext_2_t)(
ze_graph_handle_t hGraph, ///< [in] handle of the graph object
size_t* pSize, ///< [out] size of native binary in bytes
const uint8_t** pGraphNativeBinary ///< [out] double pointer to view of native binary, driver owns the mem
```ory
);
_graph_ddi_table_ext.pfnGraphInitialize(_graph); | ||
} | ||
|
||
if (properties.initStageRequired & ZE_GRAPH_STAGE_COMMAND_LIST_INITIALIZE) { |
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.
Is it possible to set both ZE_GRAPH_STAGE_INITIALIZE
and ZE_GRAPH_STAGE_COMMAND_LIST_INITIALIZE
?
If so, we can potentially call graph initialize twice.
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.
It shouldn't be possible right now. But we don't know what the future will be. We should check the bit and if it is set, just call it.
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.
If they are mutually exclusive, you can consider moving one of the if bodies into else if
branch.
But it is more like a nitpick.
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 was thinking about this but we don't know for sure in the future how this change. Since that property is represented on bits makes more sense to check the bit instead of doing an if-else. In this way driver should decide if only one is needed, both, or none.
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 the explanations 👍
_graph_ddi_table_ext.pfnGraphInitialize(_graph); | ||
} | ||
|
||
if (properties.initStageRequired & ZE_GRAPH_STAGE_COMMAND_LIST_INITIALIZE) { |
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.
If they are mutually exclusive, you can consider moving one of the if bodies into else if
branch.
But it is more like a nitpick.
Details:
Tickets: