[l0][HIP][CUDA] Prefer values over pointers-to

[ldrumm]

Flags are 32bit here and none of these implementations functions need modify their argument. Passing by reference is opaque, slower, and superfluous in this case, so let's just pass by value.

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 12.43%. Comparing base (78ef1ca) to head (4885781).
Report is 188 commits behind head on main.

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

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1457      +/-   ##
==========================================
- Coverage   14.82%   12.43%   -2.40%     
==========================================
  Files         250      241       -9     
  Lines       36220    36242      +22     
  Branches     4094     4111      +17     
==========================================
- Hits         5369     4506     -863     
- Misses      30800    31732     +932     
+ Partials       51        4      -47     

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

[hdelan]

Can you add the [CUDA][HIP][L0] tags to your PR title?

[MartinWehking]

Can you add the [CUDA][HIP][L0] tags to your PR title?

Hi @hdelan,

Is that a good idea? Adding those tags would make the PR title (and later the squashed commit title) grow very long.
Normally, a commit title should fit into an email subject line with 50 chars or less.
It would work if @ldrumm changed the title into
"[CUDA][HIP][L0] Don't use a pointer" or something like that, but that comes at the cost of descriptiveness.

[GeorgeWeb]

Can you add the [CUDA][HIP][L0] tags to your PR title?

Hi @hdelan,

Is that a good idea? Adding those tags would make the PR title (and later the squashed commit title) grow very long.

Normally, a commit title should fit into an email subject line with 50 chars or less.

It would work if @ldrumm changed the title into

"[CUDA][HIP][L0] Don't use a pointer" or something like that, but that comes at the cost of descriptiveness.

As far as I was aware unified-runtime doesn't squash on merge (as the dpcpp repo does) and so it doesn't use the title of the PR.

[hdelan]

As far as I was aware unified-runtime doesn't squash on merge (as the dpcpp repo does) and so it doesn't use the title of the PR. As long as the commits are descriptive enough it's fine.

The PR title will be used in the merge commit that is pushed by a gatekeeper. For instance:

commit ed1f8bf618c88eaabea6bde0f6c06fc265f3b49f (upstream/main, origin/main, origin/HEAD, main)
Merge: ca5c3421 69c43b45
Author: Kenneth Benzie (Benie) <[email protected]>
Date:   Tue Mar 19 21:00:20 2024 +0000

    Merge pull request #1326 from hdelan/refactor-guess-local-worksize
    
    [CUDA][HIP] Fix bug in guess local worksize funcs and improve local worksize guessing in HIP adapter

Convention says that we use these tags in describing PRs so I don't think it's controversial to add these.

It would be simple to change this PR description to fit in 50 chars although I don't know why we need to be strict with this constraint.

[jchlanda]

As a general comment, and I admit I did not check it myself, is it always safe to use 0 where nullptr used to be used? If a flag/enum happens to use 0 as a valid entry the behaviour would change.

I'm thinking about something along the lines of this:

enum class Dog { good_boy = 0, cat };

void woof(Dog *D) {
  if  (D && *D == Dog::good_boy) {

  }
}

void woofWoof(Dog D) {
  if (D == Dog::good_boy) {

  }
}

woof(nullptr);
woofWoof(0);

[ldrumm]

As a general comment, and I admit I did not check it myself, is it always safe to use 0 where nullptr used to be used? If a flag/enum happens to use 0 as a valid entry the behaviour would change.

I'm thinking about something along the lines of this:

enum class Dog { good_boy = 0, cat };

void woof(Dog *D) {
  if  (D && *D == Dog::good_boy) {

  }
}

void woofWoof(Dog D) {
  if (D == Dog::good_boy) {

  }
}

woof(nullptr);
woofWoof(0);

In general you're correct: nullptr indicates the absense of something but zero might be a valid value. However, in the specific case of ur_usm_host_flags_t and ur_usm_device_flags_t zero means "nil". Since these are internal interfaces, I'm not worried about that problem.

[ldrumm]

As far as I was aware unified-runtime doesn't squash on merge (as the dpcpp repo does) and so it doesn't use the title of the PR. As long as the commits are descriptive enough it's fine.

The PR title will be used in the merge commit that is pushed by a gatekeeper. For instance:

commit ed1f8bf618c88eaabea6bde0f6c06fc265f3b49f (upstream/main, origin/main, origin/HEAD, main)
Merge: ca5c3421 69c43b45
Author: Kenneth Benzie (Benie) <[email protected]>
Date:   Tue Mar 19 21:00:20 2024 +0000

    Merge pull request #1326 from hdelan/refactor-guess-local-worksize
    
    [CUDA][HIP] Fix bug in guess local worksize funcs and improve local worksize guessing in HIP adapter

Convention says that we use these tags in describing PRs so I don't think it's controversial to add these.

It would be simple to change this PR description to fit in 50 chars although I don't know why we need to be strict with this constraint.

I could add them, but I agree with Martin: I think prose is more important than tags that can be inferred by from the diffstat

$ git show --stat 48857812743d7e13762b5e29eb2e4bd9db824760
commit 48857812743d7e13762b5e29eb2e4bd9db824760 (ldrumm/usm-flags-by-val, usm-flags-by-val)
Author: Luke Drummond <[email protected]>
Date:   Tue Mar 19 17:03:54 2024 +0000

    Don't use a pointer where a value will do
    
    Flags are 32bit here and none of these implementations functions need
    modify their argument. Passing by reference is opaque, slower, and
    superfluous in this case, so let's just pass by value.

 source/adapters/cuda/usm.cpp       | 24 ++++++++++++------------
 source/adapters/cuda/usm.hpp       |  8 ++++----
 source/adapters/hip/usm.cpp        | 24 ++++++++++++------------
 source/adapters/hip/usm.hpp        |  8 ++++----
 source/adapters/level_zero/usm.cpp | 27 ++++++++++++---------------
 5 files changed, 44 insertions(+), 47 deletions(-)

To me there's no point adding ad-hoc metadata manually that's already available by looking at the --stat output; it's just noise.

However, I'll accept that it is currently the accepted practice, but I don't really think the currently accepted practice buys us anything.

@kbenzie I defer to you, since you're "mister manager"

[kbenzie]

However, I'll accept that it is currently the accepted practice, but I don't really think the currently accepted practice buys us anything.

The main benefit for me as a maintainer is being able to determine the content of a PR when looking at https://github.com/oneapi-src/unified-runtime/pulls - maintainance overhead is already very high on this project so any help is appriciated.

@kbenzie I defer to you, since you're "mister manager"

My preference is to add the tags, we'd probably add them before merging anyway.

[ldrumm]

My preference is to add the tags, we'd probably add them before merging anyway.

ACK

[jchlanda]

In general you're correct: nullptr indicates the absense of something but zero might be a valid value. However, in the specific case of ur_usm_host_flags_t and ur_usm_device_flags_t zero means "nil". Since these are internal interfaces, I'm not worried about that problem.

Great, thank you for explaining the use.

[ldrumm]

@oneapi-src/unified-runtime-level-zero-write can you take a look at this please?

[ldrumm]

ping

[nrspruit]

LGTM for level zero

[ldrumm]

@oneapi-src/unified-runtime-maintain please merge