Werror fixes #1206

ProGTX · 2023-12-21T09:27:08Z

A bunch of fixes to get -Werror builds to pass,
both on Linux and on Windows (/WX)

Disable verbose Windows warnings
- _CRT_SECURE_NO_WARNINGS because of std::getenv
- C4267 because of conversions from size_t to other integers
Define WIN32_LEAN_AND_MEAN and NOMINMAX on Windows
- Gets rid of some errors and speeds up the build
Convert integer CUDA objects to std::uintptr_t
before reinterpreting them to a pointer
Fixed "unused function" warning for GetHipFormatPixelSize
- There was a lot of duplication, now a single function
  called imageElementByteSize
Mark some variables as potentially unused (only used in asserts)
Other minor fixes

jchlanda

CUDA/HIP 👍

jchlanda · 2023-12-21T09:36:00Z

source/adapters/hip/memory.hpp

@@ -502,3 +502,22 @@ struct ur_mem_handle_t_ {
    }
  }
 };
+
+constexpr size_t imageElementByteSize(hipArray_Format ArrayFormat) {


Does this need to be a fee floating function? Maybe we could hide it in the cpp file behind an anonymous namespace?

This function is used by two separate .cpp files, and used to be defined 3 times (with different names) in those two files, so I figured it'd be better to have a single definition. If that's not ideal because the function is now available in the entire adapter, what would be the preffered approach? Have two separate definitions of the function in anonymous namespace, or maybe a function that's defined in one .cpp file and externed in another?

My bad, I only checked the diffs and it looked like it was only used in memory.cpp. I'd vote for one def + extern.

Single def + extern in latest version

codecov-commenter · 2023-12-21T09:41:12Z

Codecov Report

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

Comparison is base (42874bc) 15.73% compared to head (fbdb6a3) 15.73%.

Files	Patch %	Lines
test/conformance/source/environment.cpp	0.00%	1 Missing ⚠️

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

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1206      +/-   ##
==========================================
- Coverage   15.73%   15.73%   -0.01%     
==========================================
  Files         223      223              
  Lines       31478    31477       -1     
  Branches     3558     3558              
==========================================
- Hits         4954     4953       -1     
  Misses      26473    26473              
  Partials       51       51

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

PietroGhg

Native CPU LGTM, thank you

CMakeLists.txt

test/conformance/source/environment.cpp

pbalcer

Our CI workflows use UR_DEVELOPER_MODE=ON, enforcing -Werror. Are you using a different compiler? Or are these issue related to configurations we don't build (e.g., non-L0 adapters on windows)?

If it's the latter, it might be useful to create an issue to add these missing configurations so that the build doesn't get broken again.

A bunch of fixes to get `-Werror` builds to pass, both on Linux and on Windows (`/WX`) * Disable verbose Windows warnings * `_CRT_SECURE_NO_WARNINGS` because of `std::getenv` * C4267 because of conversions from `size_t` to other integers * Define `WIN32_LEAN_AND_MEAN` and `NOMINMAX` on Windows * Gets rid of some errors and speeds up the build * Convert integer CUDA objects to std::uintptr_t before reinterpreting them to a pointer * Fixed "unused function" warning for `GetHipFormatPixelSize` * There was a lot of duplication, now a single function called `imageElementByteSize` * Mark some variables as potentially unused (only used in asserts) * Other minor fixes

ProGTX · 2024-01-03T10:42:35Z

Our CI workflows use UR_DEVELOPER_MODE=ON, enforcing -Werror. Are you using a different compiler? Or are these issue related to configurations we don't build (e.g., non-L0 adapters on windows)?

If it's the latter, it might be useful to create an issue to add these missing configurations so that the build doesn't get broken again.

On Linux I built this with UR_BUILD_ADAPTER_ALL=ON, and on Windows I built CUDA, L0, and Native CPU. Which ones would need to be added to CI then?

isaacault

Bindless images LGTM

pbalcer · 2024-01-09T10:38:09Z

Which ones would need to be added to CI then?

On Linux, we are missing a Native CPU build. On Windows, we are missing CUDA, HIP, OpenCL and Native CPU.

kbenzie · 2024-01-09T11:32:17Z

On Linux, we are missing a Native CPU build. On Windows, we are missing CUDA, HIP, OpenCL and Native CPU.

The HIP adapter is not supported on Windows currently, so doubt we need to add testing for it just now. We already have #1069 for native cpu testing on Linux.

ProGTX · 2024-01-10T09:24:35Z

Friendly ping for @oneapi-src/unified-runtime-level-zero-write

nrspruit

Good for the level zero side.

Werror fixes

ProGTX requested review from a team as code owners December 21, 2023 09:27

ProGTX requested a review from jchlanda December 21, 2023 09:27

jchlanda approved these changes Dec 21, 2023

View reviewed changes

PietroGhg approved these changes Dec 21, 2023

View reviewed changes

pbalcer reviewed Dec 21, 2023

View reviewed changes

CMakeLists.txt Show resolved Hide resolved

test/conformance/source/environment.cpp Show resolved Hide resolved

PietroGhg mentioned this pull request Dec 21, 2023

[NATIVE_CPU] UR compilation error with native cpu support #1200

Closed

pbalcer approved these changes Dec 22, 2023

View reviewed changes

ProGTX force-pushed the peter/werror branch from 5ed5b13 to 77b4c1a Compare January 3, 2024 10:34

isaacault approved these changes Jan 4, 2024

View reviewed changes

Merge branch 'main' into peter/werror

fd55f66

pbalcer mentioned this pull request Jan 9, 2024

[UR][Layer] Add Sanitizer Layer #1074

Merged

ProGTX mentioned this pull request Jan 9, 2024

Add Build and CTS workflows for missing adapters on Windows #1237

Open

ProGTX added 2 commits January 9, 2024 15:13

Merge branch 'main' into peter/werror

43e5d46

Merge branch 'main' into peter/werror

1f09edf

kbenzie added the ready to merge Added to PR's which are ready to merge label Jan 10, 2024

nrspruit approved these changes Jan 10, 2024

View reviewed changes

Merge branch 'main' into peter/werror

fbdb6a3

kbenzie merged commit c63ad9b into oneapi-src:main Jan 16, 2024

ProGTX deleted the peter/werror branch January 16, 2024 11:05

kbenzie added a commit to kbenzie/unified-runtime that referenced this pull request Jan 22, 2024

Merge pull request oneapi-src#1206 from ProGTX/peter/werror

24e9815

Werror fixes

kbenzie mentioned this pull request Jan 22, 2024

Candidate for v0.8.3 release tag #1219

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Werror fixes #1206

Werror fixes #1206

Uh oh!

ProGTX commented Dec 21, 2023 •

edited

Loading

Uh oh!

jchlanda left a comment

Uh oh!

jchlanda Dec 21, 2023

Uh oh!

ProGTX Dec 21, 2023

Uh oh!

jchlanda Dec 22, 2023 •

edited

Loading

Uh oh!

ProGTX Jan 3, 2024

Uh oh!

codecov-commenter commented Dec 21, 2023 •

edited

Loading

Uh oh!

PietroGhg left a comment

Uh oh!

Uh oh!

Uh oh!

pbalcer left a comment •

edited

Loading

Uh oh!

ProGTX commented Jan 3, 2024

Uh oh!

isaacault left a comment

Uh oh!

pbalcer commented Jan 9, 2024

Uh oh!

kbenzie commented Jan 9, 2024 •

edited

Loading

Uh oh!

ProGTX commented Jan 10, 2024

Uh oh!

nrspruit left a comment

Uh oh!

Uh oh!

Werror fixes #1206

Werror fixes #1206

Uh oh!

Conversation

ProGTX commented Dec 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jchlanda left a comment

Choose a reason for hiding this comment

Uh oh!

jchlanda Dec 21, 2023

Choose a reason for hiding this comment

Uh oh!

ProGTX Dec 21, 2023

Choose a reason for hiding this comment

Uh oh!

jchlanda Dec 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ProGTX Jan 3, 2024

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Dec 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

PietroGhg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

pbalcer left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ProGTX commented Jan 3, 2024

Uh oh!

isaacault left a comment

Choose a reason for hiding this comment

Uh oh!

pbalcer commented Jan 9, 2024

Uh oh!

kbenzie commented Jan 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ProGTX commented Jan 10, 2024

Uh oh!

nrspruit left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ProGTX commented Dec 21, 2023 •

edited

Loading

jchlanda Dec 22, 2023 •

edited

Loading

codecov-commenter commented Dec 21, 2023 •

edited

Loading

pbalcer left a comment •

edited

Loading

kbenzie commented Jan 9, 2024 •

edited

Loading