[SYCL] Optimize UR 2 PI convert #8737

LukaszJobczyk · 2023-03-22T13:54:19Z

Signed-off-by: Lukasz Jobczyk lukasz.jobczyk@intel.com

sycl/plugins/unified_runtime/pi2ur.hpp

aelovikov-intel · 2023-03-23T15:55:59Z

Another option might be to implement a small constexpr map-like helper, something like https://www.youtube.com/watch?v=INn3xa4pMfg

smaslov-intel · 2023-03-23T17:54:46Z

Another option might be to implement a small constexpr map-like helper, something like https://www.youtube.com/watch?v=INn3xa4pMfg

This seems to end up doing linear search, where as "switch" will likely end up in an indirect access, which is faster.

aelovikov-intel · 2023-03-23T18:14:11Z

This seems to end up doing linear search, where as "switch" will likely end up in an indirect access, which is faster.

Yes, but then we can also optimize the pattern inside the compiler as it would be pretty straightforward.

smaslov-intel · 2023-03-23T18:20:13Z

This seems to end up doing linear search, where as "switch" will likely end up in an indirect access, which is faster.

Yes, but then we can also optimize the pattern inside the compiler as it would be pretty straightforward.

I don't see how it could potentially do better than a "switch"

smaslov-intel · 2023-03-23T18:32:14Z

sycl/plugins/unified_runtime/pi2ur.hpp

@@ -70,7 +81,7 @@ class ConvertHelper : public ReturnHelper {
 public:
  // Convert the value using a conversion map
  template <typename TypeUR, typename TypePI>
-  pi_result convert(const std::unordered_map<TypeUR, TypePI> &Map) {
+  pi_result convert(std::function<std::optional<TypePI>(TypeUR)> Func) {


Why do you need the optional stuff at all? A conversion should always be defined, and we should die at line 99 if it is not

I guess I see, you return std::nullopt when there is no conversion defined, and then die if it's conversion to bool is "false". Why not just die directly in the conversion switches directly?

I moved die as default in switch statement and remove std::optional

aelovikov-intel · 2023-03-23T18:52:30Z

I don't see how it could potentially do better than a "switch"

It could be exactly as performant as switch but more concise in the actual "mapping" part: https://godbolt.org/z/7zcrq3Wzb

smaslov-intel · 2023-03-23T23:32:09Z

I don't see how it could potentially do better than a "switch"

It could be exactly as performant as switch but more concise in the actual "mapping" part: https://godbolt.org/z/7zcrq3Wzb

I don't feel strongly about it and will let PR author to decide. But IMHO paying (presumably) lots of compile-time for (a little) better readability is not worth it. Thanks for pointing this anyways!

jandres742

@smaslov-intel @LukaszJobczyk
could we not merge this? we are in the process of merging the UR L0, and this is very big change that would force to do a big rebase and fix conflicts. Let's do it after it.

LukaszJobczyk · 2023-03-24T11:30:27Z

I don't see how it could potentially do better than a "switch"

It could be exactly as performant as switch but more concise in the actual "mapping" part: https://godbolt.org/z/7zcrq3Wzb

I don't feel strongly about it and will let PR author to decide. But IMHO paying (presumably) lots of compile-time for (a little) better readability is not worth it. Thanks for pointing this anyways!

Thanks for video about constexpr maps!
IMO switch cases look simpler

LukaszJobczyk · 2023-03-24T11:34:19Z

@smaslov-intel @LukaszJobczyk could we not merge this? we are in the process of merging the UR L0, and this is very big change that would force to do a big rebase and fix conflicts. Let's do it after it.

Why? As I see you are adding new enums to that file, what is the difference between adding to map and adding to switch/case?

jandres742 · 2023-03-29T14:10:29Z

@bader

this PR has this test failing:

********************
Failed Tests (2):
  SYCL :: AtomicRef/atomic_memory_order_acq_rel.cpp

but saw it in another PR and for me it fails also with baseline. Do you know of any issues with that test?

LukaszJobczyk · 2023-03-29T14:11:54Z

@bader

this PR has this test failing:
********************
Failed Tests (2):
  SYCL :: AtomicRef/atomic_memory_order_acq_rel.cpp
but saw it in another PR and for me it fails also with baseline. Do you know of any issues with that test?

It also fails in CUDA test suite

aelovikov-intel · 2023-03-29T15:36:24Z

We have #8847 for it.

for me it fails also with baseline

Does it happen all the time or is it flaky?

LukaszJobczyk · 2023-03-29T16:04:22Z

We have #8847 for it.

for me it fails also with baseline

Does it happen all the time or is it flaky?

Today, it happened in every force push

jandres742 · 2023-03-29T16:43:31Z

We have #8847 for it.

for me it fails also with baseline

Does it happen all the time or is it flaky?

same for me. it is very deterministic.

jandres742 · 2023-03-29T19:14:11Z

@aelovikov-intel @bader

same it is happening with:

********************
Failed Tests (1):
  SYCL :: GroupAlgorithm/reduce_sycl2020.cpp

failing also with CUDA. do we have a known issue for this also?

jandres742 · 2023-03-29T19:16:52Z

@aelovikov-intel @bader

same it is happening with:
********************
Failed Tests (1):
  SYCL :: GroupAlgorithm/reduce_sycl2020.cpp
failing also with CUDA. do we have a known issue for this also?

Seems like it is being fixed in #8860

bader · 2023-03-31T03:20:42Z

@LukaszJobczyk, could you resolve merge, conflicts, please?

LukaszJobczyk · 2023-03-31T04:26:58Z

@LukaszJobczyk, could you resolve merge, conflicts, please?

Done

bader · 2023-04-01T02:04:06Z

@LukaszJobczyk, could you resolve merge, conflicts, please?

Done

It looks like, some other PR went before this PR and we have another conflict. @LukaszJobczyk, could you take a look, please?

Signed-off-by: Lukasz Jobczyk <lukasz.jobczyk@intel.com>

LukaszJobczyk · 2023-04-03T05:08:22Z

@LukaszJobczyk, could you resolve merge, conflicts, please?

Done

It looks like, some other PR went before this PR and we have another conflict. @LukaszJobczyk, could you take a look, please?

Done

LukaszJobczyk marked this pull request as ready for review March 22, 2023 14:05

LukaszJobczyk requested a review from a team as a code owner March 22, 2023 14:05

LukaszJobczyk temporarily deployed to aws March 22, 2023 14:52 — with GitHub Actions Inactive

LukaszJobczyk temporarily deployed to aws March 22, 2023 16:28 — with GitHub Actions Inactive

smaslov-intel reviewed Mar 22, 2023

View reviewed changes

sycl/plugins/unified_runtime/pi2ur.hpp Outdated Show resolved Hide resolved

smaslov-intel reviewed Mar 23, 2023

View reviewed changes

jandres742 suggested changes Mar 24, 2023

View reviewed changes

LukaszJobczyk force-pushed the optimizeur2pi branch from dcf167b to be0efaf Compare March 24, 2023 10:01

LukaszJobczyk temporarily deployed to aws March 24, 2023 10:05 — with GitHub Actions Inactive

LukaszJobczyk force-pushed the optimizeur2pi branch from be0efaf to 4653e7f Compare March 24, 2023 10:11

LukaszJobczyk temporarily deployed to aws March 24, 2023 10:15 — with GitHub Actions Inactive

LukaszJobczyk force-pushed the optimizeur2pi branch 4 times, most recently from 06e682f to 9064adc Compare March 24, 2023 11:01

LukaszJobczyk temporarily deployed to aws March 24, 2023 11:26 — with GitHub Actions Inactive

LukaszJobczyk force-pushed the optimizeur2pi branch from 9064adc to 216054d Compare March 24, 2023 11:56

LukaszJobczyk requested a review from bader as a code owner March 24, 2023 11:56

LukaszJobczyk force-pushed the optimizeur2pi branch from 216054d to b8e49ae Compare March 24, 2023 11:56

LukaszJobczyk temporarily deployed to aws March 24, 2023 12:26 — with GitHub Actions Inactive

LukaszJobczyk force-pushed the optimizeur2pi branch from b8e49ae to d32968f Compare March 24, 2023 12:30

LukaszJobczyk temporarily deployed to aws March 24, 2023 13:15 — with GitHub Actions Inactive

LukaszJobczyk force-pushed the optimizeur2pi branch from 8bacdc2 to 2397300 Compare March 29, 2023 10:17

LukaszJobczyk temporarily deployed to aws March 29, 2023 10:48 — with GitHub Actions Inactive

LukaszJobczyk force-pushed the optimizeur2pi branch from 2397300 to 27fe1c1 Compare March 29, 2023 11:51

LukaszJobczyk temporarily deployed to aws March 29, 2023 12:25 — with GitHub Actions Inactive

LukaszJobczyk temporarily deployed to aws March 29, 2023 12:59 — with GitHub Actions Inactive

LukaszJobczyk force-pushed the optimizeur2pi branch from 27fe1c1 to 3c5ab9a Compare March 30, 2023 05:55

LukaszJobczyk temporarily deployed to aws March 30, 2023 06:22 — with GitHub Actions Inactive

LukaszJobczyk temporarily deployed to aws March 30, 2023 11:46 — with GitHub Actions Inactive

LukaszJobczyk force-pushed the optimizeur2pi branch from 3c5ab9a to 4785706 Compare March 30, 2023 14:37

LukaszJobczyk temporarily deployed to aws March 30, 2023 16:00 — with GitHub Actions Inactive

LukaszJobczyk temporarily deployed to aws March 30, 2023 17:52 — with GitHub Actions Inactive

LukaszJobczyk force-pushed the optimizeur2pi branch from 4785706 to a2b3a8e Compare March 31, 2023 04:26

LukaszJobczyk temporarily deployed to aws March 31, 2023 05:23 — with GitHub Actions Inactive

LukaszJobczyk temporarily deployed to aws March 31, 2023 07:14 — with GitHub Actions Inactive

[SYCL] Optimize UR 2 PI convert

3c787ee

Signed-off-by: Lukasz Jobczyk <lukasz.jobczyk@intel.com>

LukaszJobczyk force-pushed the optimizeur2pi branch from a2b3a8e to 3c787ee Compare April 3, 2023 05:08

LukaszJobczyk temporarily deployed to aws April 3, 2023 05:46 — with GitHub Actions Inactive

LukaszJobczyk temporarily deployed to aws April 3, 2023 06:17 — with GitHub Actions Inactive

bader merged commit 22e2ca1 into intel:sycl Apr 3, 2023

[SYCL] Optimize UR 2 PI convert #8737

[SYCL] Optimize UR 2 PI convert #8737

Uh oh!

Conversation

LukaszJobczyk commented Mar 22, 2023

Uh oh!

Uh oh!

aelovikov-intel commented Mar 23, 2023

Uh oh!

smaslov-intel commented Mar 23, 2023

Uh oh!

aelovikov-intel commented Mar 23, 2023

Uh oh!

smaslov-intel commented Mar 23, 2023

Uh oh!

smaslov-intel Mar 23, 2023

Choose a reason for hiding this comment

Uh oh!

smaslov-intel Mar 23, 2023

Choose a reason for hiding this comment

Uh oh!

LukaszJobczyk Mar 24, 2023

Choose a reason for hiding this comment

Uh oh!

aelovikov-intel commented Mar 23, 2023

Uh oh!

smaslov-intel commented Mar 23, 2023

Uh oh!

jandres742 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!

LukaszJobczyk commented Mar 24, 2023

Uh oh!

LukaszJobczyk commented Mar 24, 2023

Uh oh!

jandres742 commented Mar 29, 2023

Uh oh!

LukaszJobczyk commented Mar 29, 2023

Uh oh!

aelovikov-intel commented Mar 29, 2023

Uh oh!

LukaszJobczyk commented Mar 29, 2023

Uh oh!

jandres742 commented Mar 29, 2023

Uh oh!

jandres742 commented Mar 29, 2023

Uh oh!

jandres742 commented Mar 29, 2023

Uh oh!

bader commented Mar 31, 2023

Uh oh!

LukaszJobczyk commented Mar 31, 2023

Uh oh!

bader commented Apr 1, 2023

Uh oh!

LukaszJobczyk commented Apr 3, 2023

Uh oh!

Uh oh!

jandres742 left a comment •

edited

Loading