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][SCLA] Add sycl::aspect::ext_oneapi_private_alloca #13181

Merged
merged 6 commits into from
Apr 3, 2024

Conversation

victor-eds
Copy link
Contributor

@victor-eds victor-eds commented Mar 27, 2024

Add aspect to check for sycl_ext_oneapi_private_alloca extension support. Only SPIR-V (OpenCL and Level Zero) devices have this aspect.

In CodeGen, propagate this aspect when creating the llvm.sycl.alloca.* intrinsic.

In sycl-post-link, do not assert on unsupported targets. Instead, assume the default value for the size specialization constant is used and handle the error at runtime. SPIR-V AOT compilation should fail in the frontend, so no need to fail here.

Comment on lines +45 to +46
; CHECK-RT: %[[LENGTH:.*]] = call i16 @_Z20__spirv_SpecConstantis(i32 2, i16 1)
; CHECK-RT: {{.*}} = alloca %my_range, i16 %[[LENGTH]], align 64
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Previously missing

Comment on lines -13 to +18
@size_i64 = internal addrspace(1) constant %"class.sycl::_V1::specialization_id" { i64 10 }, align 8
@size_i32 = internal addrspace(1) constant %"class.sycl::_V1::specialization_id.0" { i32 120 }, align 4
@size_i16 = internal addrspace(1) constant %"class.sycl::_V1::specialization_id.1" { i16 1 }, align 2
@size_i64 = addrspace(1) constant %"class.sycl::_V1::specialization_id" { i64 10 }, align 8
@size_i32 = addrspace(1) constant %"class.sycl::_V1::specialization_id.0" { i32 120 }, align 4
@size_i16 = addrspace(1) constant %"class.sycl::_V1::specialization_id.1" { i16 1 }, align 2
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Needed to not drop

#include <sycl/sycl.hpp>
#include <sycl/detail/core.hpp>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also did this in all the tests

@@ -1,6 +1,5 @@
// RUN: %{build} -w -o %t.out
// RUN: echo 1 | %{run} %t.out
// UNSUPPORTED: cuda || hip
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not needed, using lit.local.cfg

@@ -0,0 +1,31 @@
// RUN: %clangxx %s -fsycl -fsycl-device-only -S -emit-llvm -o - | FileCheck %s
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Run LLVM passes to check propagation of used aspects

Copy link
Contributor

github-actions bot commented Mar 27, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

@victor-eds
Copy link
Contributor Author

victor-eds commented Mar 27, 2024

Changes span different components as we need to:

  • Add logic to propagate aspects in codegen (this would fail if no aspect regardless of target)
  • Do not fail on sycl-post-link (this would lead to semantic changes if no aspect in unsupported targets)
  • Adding aspect

Copy link
Contributor

@premanandrao premanandrao left a comment

Choose a reason for hiding this comment

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

FE changes LGTM

@victor-eds
Copy link
Contributor Author

@intel/dpcpp-tools-reviewers , @intel/llvm-reviewers-runtime, can I have a quick look here, please?

Add aspect to check for `sycl_ext_oneapi_private_alloca` extension
support. Only SPIR-V (OpenCL and Level Zero) devices have this aspect.

In CodeGen, propagate this aspect when creating the
`llvm.sycl.alloca.*` intrinsic.

In `sycl-post-link`, do not assert on unsupported targets. Instead,
assume the default value for the size specialization constant is used
and handle the error at runtime.

Signed-off-by: Victor Perez <victor.perez@codeplay.com>
Copy link
Contributor

@smanna12 smanna12 left a comment

Choose a reason for hiding this comment

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

FE changes LGTM

@victor-eds
Copy link
Contributor Author

@intel/llvm-gatekeepers can we get this one merged? CI errors look unrelated to this change.

@againull
Copy link
Contributor

againull commented Apr 3, 2024


Failed Tests (1):
SYCL :: Basic/multi_ptr_null_relational_operators.cpp

failure is unrelated and fixed in #13268

@againull againull merged commit d3c76d3 into intel:sycl Apr 3, 2024
9 of 12 checks passed
@victor-eds victor-eds deleted the alloca-aspect branch April 4, 2024 08:41
kbenzie pushed a commit to kbenzie/llvm that referenced this pull request Apr 18, 2024
Add cubemap support:
 - Allocation and freeing of cubemapped images
 - Unsampled fetching and writing, and sampled reading
 - Device queries for cubemap support
 - Testing for both unsampled and sampled cubemap examples
 - Update the spec with cubemap support

Remove `const` and `&` qualifiers from spec and implementation for
handle parameters in `write_xxx` functions.

Corresponding UR PR:
oneapi-src/unified-runtime#1433

---------

Co-authored-by: Przemek Malon <przemek.malon@codeplay.com>

Resolved Conflicts in:
- Due to not cherry-picking intel#12840
  - sycl/include/sycl/detail/pi.h
- Due to not cherry-picking intel#13181
  - sycl/include/sycl/device_aspect_macros.hpp
  - sycl/include/sycl/info/aspects.def
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants