Skip to content

[SYCL] Enabling depreacate warnings for sycl #15342

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
dyniols wants to merge 19 commits into
base: sycl
Choose a base branch
from
Open

[SYCL] Enabling depreacate warnings for sycl #15342

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
da8bc72
changes needed after reenabling SemaAvailability warnings
dyniols Sep 10, 2024
6fd74a8
formatting fix
dyniols Sep 10, 2024
9ba3942
Merge remote-tracking branch 'intel/sycl' into sema_availability_reve…
dyniols Sep 10, 2024
645a5e3
atomic alias warning fix
dyniols Sep 10, 2024
551a1fe
Merge remote-tracking branch 'intel/sycl' into sema_availability_reve…
dyniols Sep 12, 2024
a926de6
test fix
dyniols Sep 12, 2024
ae91fa2
use decoreated_global_ptr instead global_ptr
dyniols Sep 12, 2024
4f2e0f5
another formatting fix
dyniols Sep 12, 2024
17f313f
Merge remote-tracking branch 'intel/sycl' into sema_availability_reve…
dyniols Sep 13, 2024
bb43f68
Merge remote-tracking branch 'intel/sycl' into sema_availability_reve…
dyniols Sep 24, 2024
18e4443
cleanup and added comment
dyniols Sep 24, 2024
1371754
Merge remote-tracking branch 'intel/sycl' into sema_availability_reve…
dyniols Nov 5, 2024
c84bbcd
fix
dyniols Nov 7, 2024
9048252
formatting fixed
dyniols Nov 7, 2024
bac802b
Merge remote-tracking branch 'intel/sycl' into sema_availability_reve…
dyniols Nov 13, 2024
9827338
fix
dyniols Nov 13, 2024
b18f45e
formatting fixed
dyniols Nov 13, 2024
449f69b
fix
dyniols Nov 13, 2024
3fabbf8
formatting fixed
dyniols Nov 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions clang/lib/Sema/SemaAvailability.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -621,8 +621,7 @@ static void DoEmitAvailabilityWarning(Sema &S, AvailabilityResult K,
S.getTopMostPointOfInstantiation(ReferringDecl);
bool ShouldAllowWarningInSystemHeader =
InstantiationLoc != Loc &&
!S.getSourceManager().isInSystemHeader(InstantiationLoc) &&
!S.getLangOpts().SYCLIsDevice && !S.getLangOpts().SYCLIsHost;
!S.getSourceManager().isInSystemHeader(InstantiationLoc);
struct AllowWarningInSystemHeaders {
AllowWarningInSystemHeaders(DiagnosticsEngine &E,
bool AllowWarningInSystemHeaders)
Expand Down
8 changes: 4 additions & 4 deletions sycl/include/sycl/accessor.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1861,28 +1861,28 @@ class __SYCL_EBO __SYCL_SPECIAL_CLASS __SYCL_TYPE(accessor) accessor :

iterator begin() const noexcept {
return iterator::getBegin(
get_pointer(),
getPointerAdjusted(),
detail::convertToArrayOfN<AdjustedDim, 1>(getMemoryRange()),
getRange<AdjustedDim>(), getOffset<AdjustedDim>());
}

iterator end() const noexcept {
return iterator::getEnd(
get_pointer(),
getPointerAdjusted(),
detail::convertToArrayOfN<AdjustedDim, 1>(getMemoryRange()),
getRange<AdjustedDim>(), getOffset<AdjustedDim>());
}

const_iterator cbegin() const noexcept {
return const_iterator::getBegin(
get_pointer(),
getPointerAdjusted(),
detail::convertToArrayOfN<AdjustedDim, 1>(getMemoryRange()),
getRange<AdjustedDim>(), getOffset<AdjustedDim>());
}

const_iterator cend() const noexcept {
return const_iterator::getEnd(
get_pointer(),
getPointerAdjusted(),
detail::convertToArrayOfN<AdjustedDim, 1>(getMemoryRange()),
getRange<AdjustedDim>(), getOffset<AdjustedDim>());
}
Expand Down
9 changes: 7 additions & 2 deletions sycl/include/sycl/atomic.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,11 +166,11 @@ extern T __spirv_AtomicMax(std::atomic<T> *Ptr, __spv::Scope::Flag,

namespace sycl {
inline namespace _V1 {
namespace detail {

template <typename T, access::address_space addressSpace =
access::address_space::global_space>
class __SYCL2020_DEPRECATED(
"sycl::atomic is deprecated since SYCL 2020") atomic {
class atomic {
friend class atomic<T, access::address_space::global_space>;
static_assert(detail::IsValidAtomicType<T>::value,
"Invalid SYCL atomic type. Valid types are: int, "
Expand Down Expand Up @@ -400,6 +400,11 @@ T atomic_fetch_max(atomic<T, addressSpace> Object, T Operand,
memory_order MemoryOrder = memory_order::relaxed) {
return Object.fetch_max(Operand, MemoryOrder);
}
} // namespace detail
template <typename T, access::address_space addressSpace =
access::address_space::global_space>
using atomic __SYCL2020_DEPRECATED("use sycl::atomic_ref instead") =
detail::atomic<T, addressSpace>;
Copy link
Contributor

@aelovikov-intel aelovikov-intel Sep 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@dyniols dyniols Sep 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added comment explaining introduction of this alias.

Copy link
Contributor

@aelovikov-intel aelovikov-intel Sep 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@steffenlarsen steffenlarsen Sep 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@aelovikov-intel aelovikov-intel Sep 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@AaronBallman AaronBallman Oct 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

@dyniols dyniols Nov 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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));
  }

Copy link
Contributor

@aelovikov-intel aelovikov-intel Nov 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you provide a standalone godbolt link showing the issue?

Copy link
Contributor Author

@dyniols dyniols Nov 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@AaronBallman AaronBallman Nov 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


} // namespace _V1
} // namespace sycl
Expand Down
14 changes: 12 additions & 2 deletions sycl/include/sycl/detail/is_device_copyable.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,14 +100,24 @@ struct __SYCL2020_DEPRECATED("This type isn't device copyable in SYCL 2020")
IsDeprecatedDeviceCopyable<T[N]> : IsDeprecatedDeviceCopyable<T> {};

#ifdef __SYCL_DEVICE_ONLY__
// Helper trait to avoid instantiated the deprecated specializations of
// IsDeprecatedDeviceCopyable when is_device_copyable_v<T> is true.
template <typename T, typename = void>
struct IsDeprecatedDeviceCopyableIfNot2020 : std::false_type {};

template <typename T>
struct IsDeprecatedDeviceCopyableIfNot2020<
T, std::enable_if_t<!is_device_copyable_v<T>>>
: detail::IsDeprecatedDeviceCopyable<T> {};

// Checks that the fields of the type T with indices 0 to (NumFieldsToCheck -
// 1) are device copyable.
template <typename T, unsigned NumFieldsToCheck>
struct CheckFieldsAreDeviceCopyable
: CheckFieldsAreDeviceCopyable<T, NumFieldsToCheck - 1> {
using FieldT = decltype(__builtin_field_type(T, NumFieldsToCheck - 1));
static_assert(is_device_copyable_v<FieldT> ||
detail::IsDeprecatedDeviceCopyable<FieldT>::value,
detail::IsDeprecatedDeviceCopyableIfNot2020<FieldT>::value,
Copy link
Contributor

@AlexeySachkov AlexeySachkov Sep 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@aelovikov-intel aelovikov-intel Sep 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any updates on this?

Copy link
Contributor

@steffenlarsen steffenlarsen Sep 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@AlexeySachkov AlexeySachkov Oct 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"The specified type is not device copyable");
};

Expand All @@ -120,7 +130,7 @@ struct CheckBasesAreDeviceCopyable
: CheckBasesAreDeviceCopyable<T, NumBasesToCheck - 1> {
using BaseT = decltype(__builtin_base_type(T, NumBasesToCheck - 1));
static_assert(is_device_copyable_v<BaseT> ||
detail::IsDeprecatedDeviceCopyable<BaseT>::value,
detail::IsDeprecatedDeviceCopyableIfNot2020<BaseT>::value,
"The specified type is not device copyable");
};

Expand Down
3 changes: 2 additions & 1 deletion sycl/include/sycl/ext/oneapi/experimental/root_group.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,8 @@ void group_barrier(ext::oneapi::experimental::root_group<dimensions> G,
// workaround that's not intended to reduce the bar for SPIR-V modules
// acceptance, but rather make a pessimistic case work until we have full
// support for the device barrier built-in from backends.
const auto ChildGroup = ext::oneapi::experimental::this_group<dimensions>();
const auto ChildGroup =
ext::oneapi::this_work_item::get_work_group<dimensions>();
if (ChildGroup.get_group_linear_range() == 1) {
group_barrier(ChildGroup);
} else {
Expand Down
2 changes: 1 addition & 1 deletion sycl/test-e2e/Basic/queue/queue_parallel_for_generic.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// RUN: %{build} -o %t.out
// RUN: %{build} -Wno-error=deprecated-declarations -o %t.out
// RUN: %{run} %t.out

//=-queue_parallel_for_generic.cpp - SYCL queue parallel_for generic lambda-=//
Expand Down
22 changes: 12 additions & 10 deletions sycl/test-e2e/GroupAlgorithm/reduce_sycl2020.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,16 +41,18 @@ void test(const InputContainer &input, BinaryOperation binary_op,
int lid = it.get_local_id(0);
out[0] = reduce_over_group(g, in[lid], binary_op);
out[1] = reduce_over_group(g, in[lid], init, binary_op);
out[2] = joint_reduce(g, global_ptr<const InputT>(in),
global_ptr<const InputT>(in) + N, binary_op);
out[3] =
joint_reduce(g, global_ptr<const InputT>(in),
global_ptr<const InputT>(in) + N, init, binary_op);
out[4] = joint_reduce(sg, global_ptr<const InputT>(in),
global_ptr<const InputT>(in) + N, binary_op);
out[5] =
joint_reduce(sg, global_ptr<const InputT>(in),
global_ptr<const InputT>(in) + N, init, binary_op);
out[2] = joint_reduce(g, decorated_global_ptr<const InputT>(in),
decorated_global_ptr<const InputT>(in) + N,
binary_op);
out[3] = joint_reduce(g, decorated_global_ptr<const InputT>(in),
decorated_global_ptr<const InputT>(in) + N,
init, binary_op);
out[4] = joint_reduce(sg, decorated_global_ptr<const InputT>(in),
decorated_global_ptr<const InputT>(in) + N,
binary_op);
out[5] = joint_reduce(sg, decorated_global_ptr<const InputT>(in),
decorated_global_ptr<const InputT>(in) + N,
init, binary_op);
});
});
}
Expand Down
2 changes: 1 addition & 1 deletion sycl/test-e2e/InorderQueue/in_order_event_release.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
//
// RUN: %{build} -o %t.out
// RUN: %{build} -Wno-error=deprecated-declarations -o %t.out
// RUN: %{run} %t.out

// SYCL ordered queue event release shortcut test
Expand Down
2 changes: 1 addition & 1 deletion sycl/test-e2e/InorderQueue/in_order_kernels.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
//
// RUN: %{build} -o %t.out
// RUN: %{build} -Wno-error=deprecated-declarations -o %t.out
// RUN: %{run} %t.out

// SYCL ordered queue kernel shortcut test
Expand Down
9 changes: 6 additions & 3 deletions sycl/test-e2e/SubGroup/sub_group_as_vec.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,13 +54,16 @@ int main(int argc, char *argv[]) {
auto x = sg.load(&global[i]);
auto x_cv1 = sg.load<const volatile sycl::int2>(&global[i]);
auto x_cv2 = sg.load(
sycl::global_ptr<const volatile sycl::int2>(&global[i]));
global.get_multi_ptr<sycl::access::decorated::yes>() + i);

// auto x_cv2 = sg.load(
// sycl::global_ptr<const volatile sycl::int2>(&global[i]));

// Local address space
auto y = sg.load(&local[i]);
auto y_cv1 = sg.load<const volatile sycl::int2>(&local[i]);
auto y_cv2 =
sg.load(sycl::local_ptr<const volatile sycl::int2>(&local[i]));
auto y_cv2 = sg.load(
local.get_multi_ptr<sycl::access::decorated::yes>() + i);

// Store result only if same for non-cv and cv
if (utils<int, 2>::cmp_vec(x, x_cv1) &&
Expand Down
4 changes: 3 additions & 1 deletion sycl/test/warnings/sycl_2020_deprecations.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@ int main() {
sycl::multi_ptr<int, sycl::access::address_space::global_space,
sycl::access::decorated::yes>
a(nullptr);
// expected-warning@+1 {{'atomic<int, sycl::access::address_space::global_space>' is deprecated: sycl::atomic is deprecated since SYCL 2020}}
// expected-warning@+1 {{'atomic' is deprecated: use sycl::atomic_ref instead}}
sycl::atomic<int> b(a);

sycl::group<1> group = sycl::detail::Builder::createGroup<1>({8}, {4}, {1});
Expand Down Expand Up @@ -250,6 +250,7 @@ int main() {
// expected-warning@+1{{'local' is deprecated: use `local_accessor` instead}}
sycl::accessor<int, 1, sycl::access::mode::read_write, sycl::target::local>
LocalAcc(sycl::range<1>(1), CGH);
// expected-warning@sycl/pointers.hpp:33 {{'multi_ptr<int, sycl::access::address_space::local_space, sycl::access::decorated::legacy>' is deprecated: decorated::legacy multi_ptr specialization is deprecated since SYCL 2020}}
// expected-warning@+3{{'multi_ptr' is deprecated: multi_ptr construction using target::local specialized accessor is deprecated since SYCL 2020}}
sycl::multi_ptr<int, sycl::access::address_space::local_space,
sycl::access::decorated::no>
Expand All @@ -261,6 +262,7 @@ int main() {
// expected-warning@+1{{'local' is deprecated: use `local_accessor` instead}}
sycl::accessor<const int, 1, sycl::access::mode::read, sycl::target::local>
LocalConstAcc(sycl::range<1>(1), CGH);
// expected-warning@sycl/pointers.hpp:33 {{'multi_ptr<const int, sycl::access::address_space::local_space, sycl::access::decorated::legacy>' is deprecated: decorated::legacy multi_ptr specialization is deprecated since SYCL 2020}}
// expected-warning@+3{{'multi_ptr' is deprecated: multi_ptr construction using target::local specialized accessor is deprecated since SYCL 2020}}
sycl::multi_ptr<const int, sycl::access::address_space::local_space,
sycl::access::decorated::no>
Expand Down