[SYCL] Optimize UR 2 PI convert

[LukaszJobczyk]

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

[aelovikov-intel]

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

[smaslov-intel]

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]

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]

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]

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

[smaslov-intel]

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?

[LukaszJobczyk]

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

[aelovikov-intel]

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]

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]

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]

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

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

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

We have #8847 for it.

for me it fails also with baseline

Does it happen all the time or is it flaky?

[LukaszJobczyk]

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]

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]

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

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

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

[LukaszJobczyk]

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

Done

[bader]

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

[LukaszJobczyk]

@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