Update SYCL UniqueToken avoiding bitset

[masterleinad]

This corresponds to #4741. I see similar speed-ups.

[ajpowelsnl]

Hi @masterleinad -- A couple of questions: 1) will this PR be in the 3.6 release? 2) if it will be in 3.6, is it still a blocker?

[masterleinad]

@ajpowelsnl Is there a reason to remove the BlocksPromotion label here if #4741 still has it?

[masterleinad]

Hi @masterleinad -- A couple of questions: 1) will this PR be in the 3.6 release? 2) if it will be in 3.6, is it still a blocker?

I think we should treat it the same as #4741.

[ajpowelsnl]

Hi @masterleinad -- A couple of questions: 1) will this PR be in the 3.6 release? 2) if it will be in 3.6, is it still a blocker?

I think we should treat it the same as #4741.

Yes, I just spoke with Christian about it, and the unique token stuff will go in Kokkos 3.6 (including the work in this PR). Thanks for responding!

[masterleinad]

Retest this please.

[masterleinad]

Retest this please.

[masterleinad]

OpenMPTarget failing with an internal compiler error is clearly unrelated,

[dalg24]

Will approve once we clarify the question about the UniqueToken<SYCL, UniqueTokenScope::Instance> constructors.

[dalg24]

I would prefer if you split the expression in two and had idx = idx % size(); like we do in CUDA

[dalg24]

Should this one be declared explicit too? ( I know we didn't in CUDA)
Also why is it OK to only have two constructors when we had 4 in CUDA?

[masterleinad]

Cuda has

  explicit UniqueToken()
      : UniqueToken<Cuda, UniqueTokenScope::Global>(
            Kokkos::Cuda().concurrency()) {}
  explicit UniqueToken(execution_space const& arg)
      : UniqueToken<Cuda, UniqueTokenScope::Global>(
            Kokkos::Cuda().concurrency(), arg) {}
  UniqueToken(size_type max_size)
      : UniqueToken<Cuda, UniqueTokenScope::Global>(max_size) {}
  UniqueToken(size_type max_size,
              execution_space const& arg = execution_space())
      : UniqueToken<Cuda, UniqueTokenScope::Global>(max_size, arg) {}

In the current form, the first two Cuda constructors are the same as the first one here and the last two Cuda constructors are the same as the second one here. In fact, the third constructor in Cuda (and HIP) should just be removed since trying to use is would be ambiguous taking the last constructor into account.

Of course, we could argue about not using default arguments to be able to only mark the constructors taking one argument as explicit. In the end, that only matters for copy initialization with an initializer list. I agree, though, that we should make all specializations for UniqueToken have the same interface.

[masterleinad]

Again, only OpenMPTarget is failing with an internal compiler error.

[masterleinad]

Retest this please.