[SYCL] Enabling depreacate warnings for sycl

[DYNIO-INTEL]

This change reverts 8cc6e9e and addresses warnings generated by the SYCL headers as a result.

[AlexeySachkov]

This IsDeprecatedDeviceCopyable thing was introduced 3 years ago as a transition measure to temporary preserve old behavior and avoid sudden breakages for end users.

I think it is time we let it go. I will let other reviewers to decide on which path we want to take, but we have two options here:

• more drastic: drop this legacy check right away
• less drastic: drop this legacy check in preview breaking changes mode

Considering that we had a deprecation message for it for a while now, I personally think that it is fine to go with the first option and just drop this thing completely right away

[aelovikov-intel]

Any updates on this?

[steffenlarsen]

Personally I would prefer that we drop it in a separate patch, just to make it easier to bisect in case it breaks something unintentionally.

[AlexeySachkov]

Personally I would prefer that we drop it in a separate patch, just to make it easier to bisect in case it breaks something unintentionally.

I have no objections against doing that in a separate PR and I agree that it may be easier for us in a potential future bisect when some customer complains that their code doesn't compile/work anymore

[elizabethandrews]

FE change LGTM. It is strange that the original commit (reverted here) did not have a FE test though.

[DYNIO-INTEL]

@intel/llvm-reviewers-runtime Could you review?

[aelovikov-intel]

I think PR description needs to provide more high-level details about what's being done and the history about this change.

[aelovikov-intel]

This needs a comment on what issues we're avoiding by introducing this alias.

[DYNIO-INTEL]

Added comment explaining introduction of this alias.

[aelovikov-intel]

Why can't we switch accessor to use atomic_ref instead?

[steffenlarsen]

atomic is deprecated in SYCL 2020 but is still part of the specification. The interfaces in accessor that use atomic are also part of the specification and are also deprecated, but because atomic is deprecated it will cause deprecation warnings as soon as accessor is instantiated, even if the user does not intend to use the interfaces that use atomic.

[aelovikov-intel]

I'm not sure it's the right way forward. @DYNIO-INTEL , can you please extract the current pattern used in the accessor similar to the unit-test from llvm/llvm-project#70353 ? I hope it would be generic enough to fix in the clang itself (without any SYCL specifics). Once done, let @AaronBallman take a look at it.

[AaronBallman]

You may be able to use if constexpr to work around the issue, but there are no pragmas or attributes that can help here (that I'm aware of).

[DYNIO-INTEL]

I think the biggest issue is this implicit conversion function as it requires type of atomic and its instantiated for diagnostic which produces deprecated warning. Is there any way to avoid it? @AaronBallman @aelovikov-intel

template <int Dims = Dimensions,
            typename = std::enable_if_t<Dims == 0 &&
                                        AccessMode == access::mode::atomic>>
  operator
#ifdef __ENABLE_USM_ADDR_SPACE__
      atomic<DataT>
#else
      atomic<DataT, AS>
#endif
      () const {
    const size_t LinearIndex = getLinearIndex(id<AdjustedDim>());
    return Test(multi_ptr<DataT, AS, access::decorated::yes>(getQualifiedPtr() +
                                                             LinearIndex));
  }

[aelovikov-intel]

Can you provide a standalone godbolt link showing the issue?

[DYNIO-INTEL]

Sure, here it's https://godbolt.org/z/czzMx6Gze

[AaronBallman]

The only answer I have is pretty unsatisfying. I couldn't find a way to do this with if constexpr because it's really based on the declaration of the member function more than anything. The only thing I could think of is to not deprecate the class but to instead deprecate the interfaces which use the class: https://godbolt.org/z/WzY5cq86G

It's not ideal, but trying to change the behavior to not diagnose the attribute until after overload resolution would be a pretty big undertaking (as I understand it), without a clear path towards how to do it.

[aelovikov-intel]

Any updates on this?