Skip to content
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.

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

[SYCL] Throw an exception when usm_shared_allocations aspect not supported #12520

Closed
wants to merge 24 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
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
7 changes: 2 additions & 5 deletions sycl/source/detail/usm/usm_impl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -146,11 +146,8 @@ void *alignedAllocInternal(size_t Alignment, size_t Size,
}
if (Kind == alloc::shared &&
!DevImpl->has(sycl::aspect::usm_shared_allocations)) {
// TODO:: Throw an exception to conform with the specification.
// Note that many tests will have to be changed to conform with the spec
// before completing this. That is, the tests will now have to expect
// exceptions as a result of failed allocations in addition to nullptr
// being returned depending on the reason why allocation failed.
throw sycl::exception(sycl::errc::feature_not_supported,
"Device does not support USM shared allocations!");
}
void *RetVal = nullptr;
if (Size == 0)
Expand Down
3 changes: 3 additions & 0 deletions sycl/test-e2e/Annotated_arg_ptr/annotated_arg.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,9 @@ MyStruct<T> operator<<(const MyStruct<T> &lhs, const MyStruct<T> &rhs) {

int main() {
queue Q;
if (!Q.get_device().has(aspect::usm_shared_allocations)) {
return 0;
}

auto *a = malloc_shared<int>(8, Q);
auto a_ptr = annotated_arg{a};
Expand Down
1 change: 1 addition & 0 deletions sycl/test-e2e/Annotated_arg_ptr/annotated_ptr.cpp
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
// REQUIRES: usm_shared_allocations
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be aspect-usm_shared_allocations?

aspect_features = set("aspect-" + a for a in aspects)
sg_size_features = set("sg-" + s for s in sg_sizes)
features = set()
features.update(aspect_features)

Copy link
Contributor Author

@lbushi25 lbushi25 Feb 2, 2024

Choose a reason for hiding this comment

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

Yeah, I noticed that most tests were using aspect-{ASPECT_NAME} format but it seems that this format works as well. Indeed, if i remove it the tests go back to failing. Perhaps @againull can confirm that both work, that is, using aspect-usm_shared_allocations or just using usm_shared_allocations?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it works, most likely it will never be run. One can achieve the same by REQUIRES: dont-ever-run-me.

// RUN: %{build} -o %t.out
// RUN: %{run} %t.out
//
Expand Down
34 changes: 18 additions & 16 deletions sycl/test-e2e/Annotated_usm/annotated_usm_kind.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -125,25 +125,27 @@ template <typename T> void testUsmKind(sycl::queue &q) {
[&]() { return ATHost(1, q); }, [&]() { return ATHost(1, Ctx); },
[&]() { return ATAnnotated(1, dev, Ctx, alloc::host); }});

CheckUsmKindAll(
alloc::shared,
std::tuple{
[&]() { return MShared(q); }, [&]() { return MShared(dev, Ctx); },
[&]() { return MAnnotated(dev, Ctx, alloc::shared); },
[&]() { return MAnnotated(dev, Ctx, properties{usm_kind_shared}); },
[&]() { return AShared(1, q); },
[&]() { return AShared(1, dev, Ctx); },
[&]() { return AAnnotated(1, dev, Ctx, alloc::shared); },
[&]() { return TShared(q); }, [&]() { return TShared(dev, Ctx); },
[&]() { return TAnnotated(dev, Ctx, alloc::shared); },
[&]() { return TAnnotated(dev, Ctx, properties{usm_kind_shared}); },
[&]() { return ATShared(1, q); },
[&]() { return ATShared(1, dev, Ctx); },
[&]() { return ATAnnotated(1, dev, Ctx, alloc::shared); }});
if (q.get_device().has(sycl::aspect::usm_shared_allocations)) {
CheckUsmKindAll(
alloc::shared,
std::tuple{
[&]() { return MShared(q); }, [&]() { return MShared(dev, Ctx); },
[&]() { return MAnnotated(dev, Ctx, alloc::shared); },
[&]() { return MAnnotated(dev, Ctx, properties{usm_kind_shared}); },
[&]() { return AShared(1, q); },
[&]() { return AShared(1, dev, Ctx); },
[&]() { return AAnnotated(1, dev, Ctx, alloc::shared); },
[&]() { return TShared(q); }, [&]() { return TShared(dev, Ctx); },
[&]() { return TAnnotated(dev, Ctx, alloc::shared); },
[&]() { return TAnnotated(dev, Ctx, properties{usm_kind_shared}); },
[&]() { return ATShared(1, q); },
[&]() { return ATShared(1, dev, Ctx); },
[&]() { return ATAnnotated(1, dev, Ctx, alloc::shared); }});
}
}

int main() {
sycl::queue q;
testUsmKind<int>(q);
return 0;
}
}
1 change: 1 addition & 0 deletions sycl/test-e2e/Basic/group_local_memory.cpp
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
// REQUIRES: usm_shared_allocations
Copy link
Contributor

Choose a reason for hiding this comment

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

Changes like this effectively mean that we are potentially reducing validation coverage for a feature, simply because another feature doesn't work. That doesn't look right, especially considering that using USM for kernel-device communication is not the only way and using regular buffer + accessor is actually guaranteed to always work.

I think that instead of doing REQUIRES, we should do a rewrite so that tests are always launched and they do not depend on USM shared allocations (unless their purpose is to test that kind of allocations)

Copy link
Contributor Author

@lbushi25 lbushi25 Feb 2, 2024

Choose a reason for hiding this comment

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

Your comment about rewriting usm shared allocations with buffers convinced me that my PR is probably not the best way to move forward long term. That said though, this now means this JIRA and the PR will have to be blocked until all those tests are rewritten. I personally think since there are many such tests that it is better to just create JIRAs for these tests by grouping tests in the same directory together and we can probably split these JIRAs among several people to speed it up or if its not time sensitive I can just take them all in my backlog and start working one by one.

// RUN: %{build} -o %t.out
// RUN: %{run} %t.out

Expand Down
1 change: 1 addition & 0 deletions sycl/test-e2e/Basic/large-range.cpp
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
// REQUIRES: usm_shared_allocation
// Temporarily add explicit '-O2' to avoid GPU hang issue with O0 optimization.
// RUN: %{build} -fno-sycl-id-queries-fit-in-int -O2 -o %t.out
// RUN: env SYCL_PARALLEL_FOR_RANGE_ROUNDING_TRACE=1 %{run} %t.out
Expand Down
1 change: 1 addition & 0 deletions sycl/test-e2e/Basic/span.cpp
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
// REQUIRES: usm_shared_allocations
// RUN: %{build} -o %t.out
// RUN: %{run} %t.out
//
Expand Down
1 change: 1 addition & 0 deletions sycl/test-e2e/Basic/wrapped_usm_pointers.cpp
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
// REQUIRES: usm_shared_allocations
// RUN: %{build} -o %t.out
// RUN: %{run} %t.out

Expand Down
1 change: 1 addition & 0 deletions sycl/test-e2e/Complex/sycl_complex_math_test.cpp
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
// REQUIRES: usm_shared_allocations
// DEFINE: %{mathflags} = %if cl_options %{/clang:-fno-fast-math%} %else %{-fno-fast-math%}

// RUN: %{build} -fsycl-device-code-split=per_kernel %{mathflags} -o %t.out
Expand Down
1 change: 1 addition & 0 deletions sycl/test-e2e/Complex/sycl_complex_operator_test.cpp
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
// REQUIRES: usm_shared_allocations
// RUN: %{build} -fsycl-device-code-split=per_kernel -o %t.out
// RUN: %{run} %t.out

Expand Down
1 change: 1 addition & 0 deletions sycl/test-e2e/Complex/sycl_complex_pow_test.cpp
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
// REQUIRES: usm_shared_allocations
// DEFINE: %{mathflags} = %if cl_options %{/clang:-fno-fast-math%} %else %{-fno-fast-math%}

// RUN: %{build} -fsycl-device-code-split=per_kernel %{mathflags} -o %t.out
Expand Down
2 changes: 2 additions & 0 deletions sycl/test-e2e/Complex/sycl_complex_stream_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@

template <typename T> struct test_sycl_stream_operator {
bool operator()(sycl::queue &Q, cmplx<T> init) {
if (!Q.get_device().has(sycl::aspect::usm_shared_allocations))
return true;
auto *cplx_out = sycl::malloc_shared<experimental::complex<T>>(1, Q);
cplx_out[0] = experimental::complex<T>(init.re, init.im);

Expand Down
62 changes: 32 additions & 30 deletions sycl/test-e2e/DiscardEvents/discard_events_mixed_calls.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,8 @@ static constexpr int MAX_ITER_NUM2 = 10;
void TestHelper(sycl::queue Q,
const std::function<void(sycl::range<1> Range, int *Harray,
sycl::buffer<int, 1> Buf)> &Function) {
if (!Q.get_device().has(aspect::usm_shared_allocations))
return;
int *Harray = sycl::malloc_shared<int>(BUFFER_SIZE, Q);
assert(Harray != nullptr);
for (size_t i = 0; i < BUFFER_SIZE; ++i) {
Expand Down Expand Up @@ -113,36 +115,36 @@ void RunTest_USM_Accessor(sycl::queue Q) {
}

void RunTest_Accessor_USM(sycl::queue Q) {
TestHelper(
Q, [&](sycl::range<1> Range, int *Harray, sycl::buffer<int, 1> Buf) {
{
sycl::host_accessor HostAcc(Buf);
for (size_t i = 0; i < BUFFER_SIZE; ++i) {
HostAcc[i] = 0;
}
}

for (int i = 0; i < MAX_ITER_NUM1; ++i)
IfTrueIncrementBufferAndUSM(Q, Range, Harray, Buf, (i));

for (int i = 0; i < MAX_ITER_NUM2; ++i)
IfTrueIncrementUSM(Q, Range, Harray, (MAX_ITER_NUM1 + i));

Q.wait();

// check results
for (size_t i = 0; i < BUFFER_SIZE; ++i) {
int expected = MAX_ITER_NUM1 + MAX_ITER_NUM2;
assert(Harray[i] == expected);
}
{
sycl::host_accessor HostAcc(Buf, sycl::read_only);
for (size_t i = 0; i < BUFFER_SIZE; ++i) {
int expected = MAX_ITER_NUM1;
assert(HostAcc[i] == expected);
}
}
});
TestHelper(Q,
[&](sycl::range<1> Range, int *Harray, sycl::buffer<int, 1> Buf) {
{
sycl::host_accessor HostAcc(Buf);
for (size_t i = 0; i < BUFFER_SIZE; ++i) {
HostAcc[i] = 0;
}
}

for (int i = 0; i < MAX_ITER_NUM1; ++i)
IfTrueIncrementBufferAndUSM(Q, Range, Harray, Buf, (i));

for (int i = 0; i < MAX_ITER_NUM2; ++i)
IfTrueIncrementUSM(Q, Range, Harray, (MAX_ITER_NUM1 + i));

Q.wait();

// check results
for (size_t i = 0; i < BUFFER_SIZE; ++i) {
int expected = MAX_ITER_NUM1 + MAX_ITER_NUM2;
assert(Harray[i] == expected);
}
{
sycl::host_accessor HostAcc(Buf, sycl::read_only);
for (size_t i = 0; i < BUFFER_SIZE; ++i) {
int expected = MAX_ITER_NUM1;
assert(HostAcc[i] == expected);
}
}
});
}

void RunTest_Mixed(sycl::queue Q) {
Expand Down
1 change: 1 addition & 0 deletions sycl/test-e2e/DiscardEvents/discard_events_usm.cpp
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
// REQUIRES: usm_shared_allocations
// RUN: %{build} -o %t.out

// RUN: env SYCL_PI_TRACE=2 %{run} %t.out &> %t.txt ; FileCheck %s --input-file %t.txt
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
// REQUIRES: usm_shared_allocations
// RUN: %{build} -o %t.out

// RUN: env SYCL_PI_TRACE=2 %{run} %t.out &> %t.txt ; FileCheck %s --input-file %t.txt
Expand Down
1 change: 1 addition & 0 deletions sycl/test-e2e/DiscardEvents/invalid_event.cpp
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
// REQUIRES: usm_shared_allocations
// Same hang as on Basic/barrier_order.cpp tracked in
// https://github.com/intel/llvm/issues/7330.
// UNSUPPORTED: opencl && gpu
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
// REQUIRES: usm_shared_allocations
// CPU and ACC not yet supported:
// Unsupported SPIR-V module SPIRV module requires unsupported capability 6400
// REQUIRES: gpu
Expand Down
4 changes: 4 additions & 0 deletions sycl/test-e2e/GroupAlgorithm/root_group.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@ void testQueriesAndProperties() {

void testRootGroup() {
sycl::queue q;
if (!q.get_device().has(sycl::aspect::usm_shared_allocations))
return;
const auto bundle =
sycl::get_kernel_bundle<sycl::bundle_state::executable>(q.get_context());
const auto kernel = bundle.get_kernel<class RootGroupKernel>();
Expand Down Expand Up @@ -66,6 +68,8 @@ void testRootGroup() {

void testRootGroupFunctions() {
sycl::queue q;
if (!q.get_device().has(sycl::aspect::usm_shared_allocations))
return;
const auto bundle =
sycl::get_kernel_bundle<sycl::bundle_state::executable>(q.get_context());
const auto kernel = bundle.get_kernel<class RootGroupFunctionsKernel>();
Expand Down
3 changes: 2 additions & 1 deletion sycl/test-e2e/InOrderEventsExt/get_last_event.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,8 @@ int main() {
Failed += Check(Q, "host_task", [&]() {
return Q.submit([&](sycl::handler &CGH) { CGH.host_task([]() {}); });
});

if (!Q.get_device().has(sycl::aspect::usm_shared_allocations))
return Failed;
constexpr size_t N = 64;
int *Data1 = sycl::malloc_shared<int>(N, Q);
int *Data2 = sycl::malloc_shared<int>(N, Q);
Expand Down
1 change: 1 addition & 0 deletions sycl/test-e2e/InOrderEventsExt/set_external_event.cpp
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
// REQUIRES: usm_shared_allocations
// RUN: %{build} -o %t.out
// RUN: %{run} %t.out

Expand Down
1 change: 1 addition & 0 deletions sycl/test-e2e/KernelAndProgram/disable-caching.cpp
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
// REQUIRES: usm_shared_allocations
// This test ensures created program/kernels are not retained
// if and only if caching is disabled.

Expand Down
1 change: 1 addition & 0 deletions sycl/test-e2e/KernelFusion/sync_two_queues_event_dep.cpp
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
// REQUIRES: usm_shared_allocations
// For this test, complete_fusion must be supported.
// RUN: %{build} -o %t.out
// RUN: env SYCL_RT_WARNING_LEVEL=1 %{run} %t.out 2>&1 | FileCheck %s
Expand Down
1 change: 1 addition & 0 deletions sycl/test-e2e/KernelFusion/sync_usm_mem_op.cpp
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
// REQUIRES: usm_shared_allocations
// RUN: %{build} -fsycl-embed-ir -o %t.out
// RUN: env SYCL_RT_WARNING_LEVEL=1 %{run} %t.out 2>&1 | FileCheck %s

Expand Down
3 changes: 2 additions & 1 deletion sycl/test-e2e/Reduction/reduction_internal.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,8 @@ static void test(RedStorage &Storage, RangeTy Range) {
cgh, Range, ext::oneapi::experimental::empty_properties_t{}, RedSycl,
[=](auto Item, auto &Red) { Red.combine(T{1}); });
}).wait();

if (!q.get_device().has(aspect::usm_shared_allocations))
return;
auto *Result = malloc_shared<T>(1, q);
q.submit([&](handler &cgh) {
auto RedAcc = GetRedAcc(cgh);
Expand Down
1 change: 1 addition & 0 deletions sycl/test-e2e/Reduction/reduction_range_item.cpp
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
// REQUIRES: usm_shared_allocations
// RUN: %{build} -o %t.out
// RUN: %{run} %t.out

Expand Down
1 change: 1 addition & 0 deletions sycl/test-e2e/Reduction/reduction_span.cpp
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
// REQUIRES: usm_shared_allocations
// RUN: %{build} -o %t.out
// RUN: %{run} %t.out

Expand Down
1 change: 1 addition & 0 deletions sycl/test-e2e/Reduction/reduction_span_pack.cpp
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
// REQUIRES: usm_shared_allocations
// RUN: %{build} -o %t.out
// RUN: %{run} %t.out
//
Expand Down
1 change: 1 addition & 0 deletions sycl/test-e2e/Regression/exclusive-scan-char-short.cpp
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
// REQUIRES: usm_shared_allocations
// RUN: %{build} -o %t.out
// RUN: %{run} %t.out

Expand Down
1 change: 1 addition & 0 deletions sycl/test-e2e/Regression/group_local_linear_id.cpp
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
// REQUIRES: usm_shared_allocations
// RUN: %{build} -o %t.out
// RUN: %{run} %t.out

Expand Down
1 change: 1 addition & 0 deletions sycl/test-e2e/Regression/half_operators.cpp
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
// REQUIRES: usm_shared_allocations
// REQUIRES: gpu
// RUN: %{build} -fsycl-device-code-split=per_kernel -o %t.out
// RUN: %{run} %t.out
Expand Down
1 change: 1 addition & 0 deletions sycl/test-e2e/Regression/pf-wg-atomic64.cpp
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
// REQUIRES: usm_shared_allocations
// DISABLED: aspect-atomic64
// RUN: %{build} -o %t.out
// RUN: %{run} %t.out
Expand Down
1 change: 1 addition & 0 deletions sycl/test-e2e/Regression/range-rounding-this-id.cpp
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
// REQUIRES: usm_shared_allocations
// This test ensures that this_id returns the correct value
// even when a kernel is wrapped in a range rounding kernel.
// RUN: %{build} -o %t.out
Expand Down
1 change: 1 addition & 0 deletions sycl/test-e2e/Regression/reduction_64bit_atomic64.cpp
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
// REQUIRES: usm_shared_allocations
// REQUIRES: aspect-atomic64
// RUN: %{build} -o %t.out
//
Expand Down
Loading
Loading