Tidy and move common adapter code to ur_common #997

callumfare · 2023-10-25T15:58:19Z

Move common code from ur.cpp and ur.hpp to the ur_common static library.
Tidy up this shared code at the same time - mostly formatting to match the adapter code, and putting helper classes in namespaces to clarify that they aren't part of UR itself but just implementation details.
Fix missing includes in the adapters as a result of moving from ur.hpp to ur_util.hpp
Also fix unused definition of GetHipFormatPixelSize on HIP < 5.6 as this came up when I was testing locally

pbalcer · 2023-10-26T07:42:41Z

adapters already link with common. Any reason this new stuff can't be part of that library? there's already an ur_util.hpp file.

kbenzie · 2023-10-26T10:44:02Z

adapters already link with common. Any reason this new stuff can't be part of that library? there's already an ur_util.hpp file.

That might make more sense, yeah.

callumfare · 2023-10-26T13:26:43Z

adapters already link with common. Any reason this new stuff can't be part of that library? there's already an ur_util.hpp file.

I'm waiting for the CI to rerun on the latest commit, but after moving this into common the quick fuzztest job was failing because of ODR violations. I think because common is an interface library the definitions in ur_adapter_util.cpp are being pulled in multiple times. Either I can revert and keep the adapter utils as a separate static library, or possibly just remove the entire cpp file and make it header-only. PrintTrace and the platform cache are only used by the Level Zero adapter now anyway so could be moved there.

kbenzie · 2023-10-26T15:18:54Z

I think because common is an interface library the definitions in ur_adapter_util.cpp are being pulled in multiple times.

Sounds like it should be a static library instead.

kbenzie · 2023-10-26T15:20:25Z

In fact, it already is on the main branch https://github.com/oneapi-src/unified-runtime/blob/main/source/common/CMakeLists.txt#L9

fabiomestre · 2023-12-05T16:52:40Z

I have updated the target branch of this PR from the adapters branch to the main branch.
Development in UR is moving back to main. The adapters branch will soon be deleted.

pbalcer

please rebase.

codecov-commenter · 2023-12-12T16:44:06Z

Codecov Report

Attention: 76 lines in your changes are missing coverage. Please review.

Comparison is base (8007b22) 15.59% compared to head (800bf88) 15.57%.
Report is 2 commits behind head on main.

Files	Patch %	Lines
source/common/ur_util.hpp	0.00%	58 Missing ⚠️
...ource/loader/layers/sanitizer/asan_interceptor.cpp	0.00%	13 Missing ⚠️
...ource/loader/layers/sanitizer/asan_interceptor.hpp	0.00%	4 Missing ⚠️
source/common/ur_util.cpp	92.85%	1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #997      +/-   ##
==========================================
- Coverage   15.59%   15.57%   -0.02%     
==========================================
  Files         232      231       -1     
  Lines       32089    32087       -2     
  Branches     3638     3638              
==========================================
- Hits         5003     4999       -4     
- Misses      27036    27037       +1     
- Partials       50       51       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

callumfare · 2023-12-13T08:55:00Z

I've rebased now. Note that I had to add /D_CRT_SECURE_NO_WARNINGS to ur_common on Windows, which was previously needed in the L0 adapter. I also had to add /DNOMINMAX to the L0 adapter as a result of includes moving around.

source/common/ur_util.cpp

PietroGhg

native cpu LGTM, thank you

isaacault

source/adapters/*/image.cpp changes LGTM.

EwanC

command-buffer changes LGTM

callumfare · 2024-01-09T15:02:04Z

@oneapi-src/unified-runtime-cuda-write @oneapi-src/unified-runtime-hip-write @oneapi-src/unified-runtime-level-zero-write Please review when you get a chance

GeorgeWeb

CUDA and HIP changes after refactor look good to me.

ldrumm · 2024-01-09T18:36:29Z

source/adapters/cuda/event.hpp

@@ -10,7 +10,7 @@
 #pragma once

 #include <cuda.h>
-#include <ur/ur.hpp>
+#include <ur_api.h>


There is a distinction between the #include <ur_api.h> (h-char-sequence) and #include "ur_api.h" (q-char-sequence). Though the c99 standard (and by reference c++11 - c++20) leave latitude for interpretation of these two forms, it's standard practise to use the "q" form for internal includes and the "h" form for system includes (see e.g. llvm).

Since ur_api.h is part of the present project, I believe all instances should the quoted form. Such a general cleanup certainly belongs in a separate patch, but for all changes introduced changes here, I think it makes sense to use q-char uniformly

ldrumm · 2024-01-09T18:38:28Z

source/adapters/cuda/platform.hpp

@@ -9,7 +9,8 @@
 //===----------------------------------------------------------------------===//
 #pragma once

-#include <ur/ur.hpp>
+#include <memory>
+#include <ur_api.h>


Are the headers not ordered hierarchically e.g local, 3rd-party, standard-library or similar?

ldrumm · 2024-01-09T18:39:50Z

source/adapters/hip/memory.cpp

@@ -15,6 +15,7 @@

 namespace {

+#if HIP_VERSION >= 50600000


Does this belong in a separate patch?

nrspruit

LGTM for level zero

callumfare force-pushed the callum/adapter_util_lib branch from 2750320 to 159963b Compare October 25, 2023 15:59

kbenzie mentioned this pull request Oct 26, 2023

[HIP] Hip adapter multi dev ctx #999

Merged

callumfare force-pushed the callum/adapter_util_lib branch 2 times, most recently from 698f581 to ac29419 Compare October 26, 2023 11:57

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

pbalcer reviewed Dec 11, 2023

View reviewed changes

callumfare force-pushed the callum/adapter_util_lib branch 7 times, most recently from 471c118 to 4f70a6b Compare December 12, 2023 16:31

callumfare force-pushed the callum/adapter_util_lib branch from 4f70a6b to 482eec0 Compare December 12, 2023 16:52

callumfare changed the title ~~Tidy and move common code to separate adapter_util static library~~ Tidy and move common adapter code to ur_common Dec 13, 2023

callumfare marked this pull request as ready for review December 13, 2023 08:55

callumfare requested review from a team as code owners December 13, 2023 08:55

aarongreig reviewed Dec 13, 2023

View reviewed changes

source/common/ur_util.cpp Outdated Show resolved Hide resolved

aarongreig approved these changes Dec 13, 2023

View reviewed changes

pbalcer approved these changes Dec 13, 2023

View reviewed changes

source/common/ur_util.cpp Outdated Show resolved Hide resolved

source/common/ur_util.cpp Outdated Show resolved Hide resolved

PietroGhg approved these changes Dec 13, 2023

View reviewed changes

callumfare force-pushed the callum/adapter_util_lib branch 3 times, most recently from 8e9b6d0 to 3424ed7 Compare December 13, 2023 12:11

isaacault approved these changes Dec 13, 2023

View reviewed changes

EwanC approved these changes Dec 13, 2023

View reviewed changes

callumfare force-pushed the callum/adapter_util_lib branch 2 times, most recently from c0b5155 to d867d48 Compare January 9, 2024 12:54

GeorgeWeb approved these changes Jan 9, 2024

View reviewed changes

ldrumm requested changes Jan 9, 2024

View reviewed changes

nrspruit approved these changes Jan 10, 2024

View reviewed changes

Tidy and move common adapter code to ur_common

800bf88

callumfare force-pushed the callum/adapter_util_lib branch from d867d48 to 800bf88 Compare January 17, 2024 14:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tidy and move common adapter code to ur_common #997

Tidy and move common adapter code to ur_common #997

callumfare commented Oct 25, 2023 •

edited

Loading

pbalcer commented Oct 26, 2023

kbenzie commented Oct 26, 2023

callumfare commented Oct 26, 2023

kbenzie commented Oct 26, 2023

kbenzie commented Oct 26, 2023

fabiomestre commented Dec 5, 2023

pbalcer left a comment

codecov-commenter commented Dec 12, 2023 •

edited

Loading

callumfare commented Dec 13, 2023

PietroGhg left a comment

isaacault left a comment

EwanC left a comment

callumfare commented Jan 9, 2024

GeorgeWeb left a comment

ldrumm Jan 9, 2024

ldrumm Jan 9, 2024

ldrumm Jan 9, 2024

nrspruit left a comment

Tidy and move common adapter code to ur_common #997

Are you sure you want to change the base?

Tidy and move common adapter code to ur_common #997

Conversation

callumfare commented Oct 25, 2023 • edited Loading

pbalcer commented Oct 26, 2023

kbenzie commented Oct 26, 2023

callumfare commented Oct 26, 2023

kbenzie commented Oct 26, 2023

kbenzie commented Oct 26, 2023

fabiomestre commented Dec 5, 2023

pbalcer left a comment

Choose a reason for hiding this comment

codecov-commenter commented Dec 12, 2023 • edited Loading

Codecov Report

callumfare commented Dec 13, 2023

PietroGhg left a comment

Choose a reason for hiding this comment

isaacault left a comment

Choose a reason for hiding this comment

EwanC left a comment

Choose a reason for hiding this comment

callumfare commented Jan 9, 2024

GeorgeWeb left a comment

Choose a reason for hiding this comment

ldrumm Jan 9, 2024

Choose a reason for hiding this comment

ldrumm Jan 9, 2024

Choose a reason for hiding this comment

ldrumm Jan 9, 2024

Choose a reason for hiding this comment

nrspruit left a comment

Choose a reason for hiding this comment

callumfare commented Oct 25, 2023 •

edited

Loading

codecov-commenter commented Dec 12, 2023 •

edited

Loading