Refactor Level Zero Adapter to use new logger #1032

lplewa · 2023-11-02T14:43:35Z

No description provided.

source/adapters/level_zero/adapter.hpp

source/adapters/level_zero/command_buffer.cpp

PatKamin · 2023-11-06T08:22:58Z

source/adapters/level_zero/adapter.cpp

@@ -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__);


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.

PatKamin · 2023-11-06T08:25:03Z

source/adapters/level_zero/command_buffer.cpp

-          WG[0], WG[1], WG[2]);
+      logger::debug("urCommandBufferAppendKernelLaunchExp: using computed WG "
+                    "size = {{{}, "
+                    "{}, {}}}\n",


Is the double newline (one explicit and one aded by the logger) intended?

PatKamin · 2023-11-06T08:31:57Z

source/adapters/level_zero/context.cpp

@@ -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__);


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.

Why do you think that it will not work?

PatKamin · 2023-11-06T08:32:41Z

source/adapters/level_zero/device.cpp

@@ -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);


Extra newline, is it needed?

PatKamin · 2023-11-06T08:36:29Z

source/adapters/level_zero/event.cpp

@@ -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__);


Remove an extra newline.

I believe \n is still here (I hope Patryk was talking about this, not something else)

Yes, that is what I meant.

This time for sure - Done

PatKamin · 2023-11-06T08:40:08Z

source/adapters/level_zero/platform.cpp

@@ -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");


Sounds like at least a warning message, not debug.

PatKamin · 2023-11-06T08:43:45Z

source/adapters/level_zero/sampler.cpp

-      urPrint("urSamplerCreate: unsupported "
-              "UR_SAMPLER_PROPERTIES_ADDRESSING_MODEE "
-              "value\n");
+      logger ::error("urSamplerCreate: unsupported "


nit: Remove a whitespace after the namespace name.

PatKamin · 2023-11-08T07:52:11Z

Why do you think that it will not work?

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?

lplewa · 2023-11-08T15:03:53Z

Why do you think that it will not work?

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?

Technically it is compiler specific, but i tested few compilers and it's allways not mangled
I think this example will make better job as an example.

PatKamin · 2023-11-09T07:03:04Z

source/adapters/level_zero/kernel.cpp

@@ -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",


Remove \n

PatKamin · 2023-11-09T07:03:12Z

source/adapters/level_zero/kernel.cpp

@@ -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",


Remove \n

PatKamin · 2023-11-09T07:05:26Z

source/adapters/level_zero/platform.cpp

@@ -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.");


Remove the WARNING word as the logger should provide this information.

PatKamin · 2023-11-09T07:11:12Z

source/adapters/level_zero/memory.cpp

@@ -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__);


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?

source/adapters/level_zero/event.cpp

lukaszstolarczuk

👍

EwanC

I've not evaluated the overall change to use the logger framework, but changes to source/adapters/level_zero/command_buffer.cpp look consistent

isaacault

source/adapters/level_zero/image.cpp LGTM

przemektmalon

image.cpp changes LGTM

jandres742 · 2023-11-15T22:09:40Z

@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.

kswiecicki · 2023-11-22T13:49:03Z

@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

lplewa · 2024-02-08T16:08:37Z

I spited this PR to two PR
First part is here:
#1327
and when it will be merged i will rebase this PR to include only changes in levelzero adapter. This will allow me to create PR with changes in other adapters without dependencies, to this PR

lplewa · 2024-02-20T11:12:21Z

I created an octopus merge b45470f of all logs changes (#1359; #1347; #1345; #1342; #1032) and tested integration with intel/llvm repository

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>

* oneapi-src/unified-runtime#1032 * oneapi-src/unified-runtime#1183 * oneapi-src/unified-runtime#1243

lukaszstolarczuk reviewed Nov 2, 2023

View reviewed changes

source/adapters/level_zero/adapter.hpp Show resolved Hide resolved

source/adapters/level_zero/command_buffer.cpp Outdated Show resolved Hide resolved

source/adapters/level_zero/command_buffer.cpp Outdated Show resolved Hide resolved

PatKamin reviewed Nov 6, 2023

View reviewed changes

lplewa force-pushed the l0-log branch 2 times, most recently from 8f94c57 to b817812 Compare November 6, 2023 15:35

lplewa force-pushed the l0-log branch from b817812 to eea672f Compare November 8, 2023 15:49

PatKamin reviewed Nov 9, 2023

View reviewed changes

lplewa force-pushed the l0-log branch 2 times, most recently from c814752 to a325bef Compare November 9, 2023 15:15

lplewa marked this pull request as ready for review November 9, 2023 15:27

lplewa requested review from a team as code owners November 9, 2023 15:27

lplewa force-pushed the l0-log branch 2 times, most recently from 4a06c96 to 953c95c Compare November 9, 2023 15:57

PatKamin approved these changes Nov 13, 2023

View reviewed changes

lplewa force-pushed the l0-log branch from 3ff74bd to 4127b1a Compare November 13, 2023 16:02

lukaszstolarczuk reviewed Nov 13, 2023

View reviewed changes

source/adapters/level_zero/event.cpp Outdated Show resolved Hide resolved

source/adapters/level_zero/event.cpp Show resolved Hide resolved

PatKamin mentioned this pull request Nov 14, 2023

[SYCL][UR][L0] Access UMF pool handles through usm::pool_manager #905

Open

lplewa force-pushed the l0-log branch from 4127b1a to e96b39d Compare November 14, 2023 16:08

lukaszstolarczuk reviewed Nov 14, 2023

View reviewed changes

EwanC approved these changes Nov 14, 2023

View reviewed changes

lplewa force-pushed the l0-log branch from e96b39d to 38babb6 Compare November 14, 2023 17:47

isaacault approved these changes Nov 15, 2023

View reviewed changes

przemektmalon approved these changes Nov 15, 2023

View reviewed changes

lplewa requested a review from a team as a code owner November 24, 2023 16:44

fabiomestre changed the base branch from adapters to main December 5, 2023 16:51

pbalcer added the ready to merge Added to PR's which are ready to merge label Feb 8, 2024

lplewa force-pushed the l0-log branch from 5e108ea to ce6d821 Compare February 8, 2024 14:56

lplewa force-pushed the l0-log branch 3 times, most recently from 3879497 to 69160ae Compare February 19, 2024 14:27

This was referenced Feb 20, 2024

Refactor hip adapter to new logger #1359

Merged

Refactor opencl adapter to new logger #1347

Merged

Refactor native cpu adapter to new logger #1345

Merged

Refactor cuda adapter to new logger #1342

Merged

lplewa force-pushed the l0-log branch from 69160ae to 412aa2f Compare February 20, 2024 11:23

lplewa changed the title ~~change urprint to the new logger framework~~ Refactor Level Zero Adapter to use new logger Feb 20, 2024

lplewa force-pushed the l0-log branch from 412aa2f to 8e383e3 Compare February 22, 2024 13:05

lplewa mentioned this pull request Feb 22, 2024

Document adapter logging environment variables #1376

Merged

lplewa force-pushed the l0-log branch from 8e383e3 to 4a5efaf Compare February 28, 2024 13:54

fabiomestre mentioned this pull request Feb 28, 2024

[SPEC] Add urAdapterSetLoggingCallback entrypoint fabiomestre/unified-runtime#1

Closed

lplewa force-pushed the l0-log branch 3 times, most recently from 7c34115 to ae5d576 Compare March 19, 2024 16:28

kbenzie added the level-zero L0 adapter specific issues label Apr 3, 2024

kbenzie force-pushed the l0-log branch from ae5d576 to 7f6dfb9 Compare April 22, 2024 11:28

github-actions bot added images UR images command-buffer Command Buffer feature addition/changes/specification labels Apr 22, 2024

kbenzie force-pushed the l0-log branch from 7f6dfb9 to 8966d27 Compare April 22, 2024 11:55

Refactor Level Zero Adapter to use new logger

5dccce5

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>

kbenzie force-pushed the l0-log branch from 8966d27 to 5dccce5 Compare April 22, 2024 11:57

kbenzie merged commit ce78e15 into oneapi-src:main Apr 22, 2024
51 checks passed

kbenzie mentioned this pull request Apr 22, 2024

[UR] Bump main tag to 31d0fe15 intel/llvm#13511

Merged

steffenlarsen pushed a commit to intel/llvm that referenced this pull request Apr 23, 2024

[UR] Bump main tag to 31d0fe15 (#13511)

96b07cf

* oneapi-src/unified-runtime#1032 * oneapi-src/unified-runtime#1183 * oneapi-src/unified-runtime#1243

Refactor Level Zero Adapter to use new logger #1032

Refactor Level Zero Adapter to use new logger #1032

Conversation

lplewa commented Nov 2, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PatKamin commented Nov 8, 2023

lplewa commented Nov 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukaszstolarczuk left a comment

Choose a reason for hiding this comment

EwanC left a comment • edited Loading

Choose a reason for hiding this comment

isaacault left a comment

Choose a reason for hiding this comment

przemektmalon left a comment

Choose a reason for hiding this comment

jandres742 commented Nov 15, 2023

kswiecicki commented Nov 22, 2023

lplewa commented Feb 8, 2024

lplewa commented Feb 20, 2024

EwanC left a comment •

edited

Loading