-
Notifications
You must be signed in to change notification settings - Fork 113
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
Refactor Level Zero Adapter to use new logger #1032
Conversation
@@ -184,7 +184,7 @@ UR_APIEXPORT ur_result_t UR_APICALL urAdapterGetLastError( | |||
std::ignore = Adapter; | |||
std::ignore = Message; | |||
std::ignore = Error; | |||
urPrint("[UR][L0] %s function not implemented!\n", __FUNCTION__); | |||
logger::error("[UR][L0] {} function not implemented!", __FUNCTION__); |
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.
As the name of the logger is level_zero
, the beginning of this logger output will look like that, I think: <level_zero>[ERROR][UR][L0]
. No need for double level zero info, I suppose.
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.
Done
WG[0], WG[1], WG[2]); | ||
logger::debug("urCommandBufferAppendKernelLaunchExp: using computed WG " | ||
"size = {{{}, " | ||
"{}, {}}}\n", |
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 the double newline (one explicit and one aded by the logger) intended?
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.
Done
@@ -174,7 +175,7 @@ UR_APIEXPORT ur_result_t UR_APICALL urContextSetExtendedDeleter( | |||
std::ignore = Context; | |||
std::ignore = Deleter; | |||
std::ignore = UserData; | |||
urPrint("[UR][L0] %s function not implemented!\n", __FUNCTION__); | |||
logger::error("[UR][L0] {} function not implemented!", __FUNCTION__); |
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.
Please, test these __FUNCTION__
prints with different compilers, perhaps create a test case for such a print to verify if this works with compilers tested in UR CI.
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.
Why do you think that it will not work?
@@ -980,7 +981,7 @@ ur_result_t ur_device_handle_t_::initialize(int SubSubDeviceOrdinal, | |||
if (numQueueGroups == 0) { | |||
return UR_RESULT_ERROR_UNKNOWN; | |||
} | |||
urPrint("NOTE: Number of queue groups = %d\n", numQueueGroups); | |||
logger::error("NOTE: Number of queue groups = {}\n", numQueueGroups); |
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.
Extra newline, is it needed?
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.
Done
source/adapters/level_zero/event.cpp
Outdated
@@ -742,7 +741,7 @@ UR_APIEXPORT ur_result_t UR_APICALL urEventSetCallback( | |||
std::ignore = ExecStatus; | |||
std::ignore = Notify; | |||
std::ignore = UserData; | |||
urPrint("[UR][L0] %s function not implemented!\n", __FUNCTION__); | |||
logger::error("[UR][L0] {} function not implemented!\n", __FUNCTION__); |
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.
Remove an extra newline.
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.
Done
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 believe \n
is still here (I hope Patryk was talking about this, not something else)
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.
Yes, that is what I meant.
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.
This time for sure - Done
@@ -69,7 +70,7 @@ UR_APIEXPORT ur_result_t UR_APICALL urPlatformGet( | |||
} | |||
|
|||
if (ZeResult != ZE_RESULT_SUCCESS) { | |||
urPrint("zeInit: Level Zero initialization failure\n"); | |||
logger::debug("zeInit: Level Zero initialization failure"); |
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.
Sounds like at least a warning message, not debug.
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.
Done
urPrint("urSamplerCreate: unsupported " | ||
"UR_SAMPLER_PROPERTIES_ADDRESSING_MODEE " | ||
"value\n"); | ||
logger ::error("urSamplerCreate: unsupported " |
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.
nit: Remove a whitespace after the namespace name.
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.
Dpne
8f94c57
to
b817812
Compare
I'm not sure if it will print a kind of "pretty" name of the function. I mean not "heavily" mangled in such a way that it's uncertain what the name of the function is. Could you paste some example logs? |
@@ -482,8 +484,8 @@ UR_APIEXPORT ur_result_t UR_APICALL urKernelGetInfo( | |||
return UR_RESULT_ERROR_UNKNOWN; | |||
} | |||
default: | |||
urPrint("Unsupported ParamName in urKernelGetInfo: ParamName=%d(0x%x)\n", | |||
ParamName, ParamName); | |||
logger::error("Unsupported ParamName in urKernelGetInfo: ParamName={}\n", |
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.
Remove \n
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.
Done
@@ -541,8 +543,8 @@ UR_APIEXPORT ur_result_t UR_APICALL urKernelGetGroupInfo( | |||
return ReturnValue(uint32_t{Kernel->ZeKernelProperties->privateMemSize}); | |||
} | |||
default: { | |||
urPrint("Unknown ParamName in urKernelGetGroupInfo: ParamName=%d(0x%x)\n", | |||
ParamName, ParamName); | |||
logger::error("Unknown ParamName in urKernelGetGroupInfo: ParamName=%d\n", |
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.
Remove \n
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.
Done
@@ -48,7 +48,8 @@ UR_APIEXPORT ur_result_t UR_APICALL urPlatformGet( | |||
} | |||
|
|||
if (getenv("SYCL_ENABLE_PCI") != nullptr) { | |||
urPrint("WARNING: SYCL_ENABLE_PCI is deprecated and no longer needed.\n"); | |||
logger::warning( | |||
"WARNING: SYCL_ENABLE_PCI is deprecated and no longer needed."); |
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.
Remove the WARNING
word as the logger should provide this information.
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.
Done
@@ -2020,7 +2022,7 @@ UR_APIEXPORT ur_result_t UR_APICALL urMemImageGetInfo( | |||
std::ignore = PropSize; | |||
std::ignore = ImgInfo; | |||
std::ignore = PropSizeRet; | |||
urPrint("[UR][L0] %s function not implemented!\n", __FUNCTION__); | |||
logger::debug("{} function not implemented!", __FUNCTION__); |
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.
Some of this kind of messages (function not implemented
) are of the debug
type whereas most are of the error
type (in other files, too). Shouldn't they be unified and have the same log level across the level zero?
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.
Done
c814752
to
a325bef
Compare
4a06c96
to
953c95c
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.
👍
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've not evaluated the overall change to use the logger framework, but changes to source/adapters/level_zero/command_buffer.cpp
look consistent
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.
source/adapters/level_zero/image.cpp
LGTM
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.
image.cpp
changes LGTM
@lplewa : do we have intel/llvm PR testing this? I want to make sure there are no sycl tests relying on those strings. If so, we would need to have the intel/llvm PR to modify also the tests. |
@lplewa the SYCL testing process for adapter changes is available here: https://github.com/oneapi-src/unified-runtime/blob/main/scripts/core/CONTRIB.rst#adapter-change-process |
I spited this PR to two PR |
3879497
to
69160ae
Compare
7c34115
to
ae5d576
Compare
This commit refactors the Level Zero adapter to adopt the new logger introduced in d9cd223 (Integrate logger with library, 2023-02-03). Signed-off-by: Łukasz Plewa <lukasz.plewa@intel.com>
No description provided.