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][USM] Initial Support for USM #256

Merged
merged 6 commits into from
Jul 1, 2019
Merged

[SYCL][USM] Initial Support for USM #256

merged 6 commits into from
Jul 1, 2019

Conversation

jbrodman
Copy link
Contributor

Modifies clang to supports pointers.
Adds pointers to the Integration header object types in clang/sycl rt.
Fixes a couple of tests to work properly with these changes.

Copy link
Contributor

@bader bader left a comment

Choose a reason for hiding this comment

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

This pull requests contains 8 commits and most of them are merge commits.
Please, make a single commit in PR, rebase you branch on the tip of the sycl branch and resolve the conflict.

@@ -1,4 +1,4 @@
// RUN: %clang -std=c++11 -fsycl %s -o %t.out -lstdc++ -lOpenCL -lsycl
// RUN: env ENABLE_INFER_AS=1 %clang -std=c++11 -fsycl %s -o %t.out -lstdc++ -lOpenCL -lsycl
Copy link
Contributor

Choose a reason for hiding this comment

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

AFAIK, this variable is not needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is. Adding pointers requires this mode to be enabled in the compiler. Otherwise conversations between global/generic aren't properly handled.

Copy link
Contributor

Choose a reason for hiding this comment

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

@asavonic, could you confirm that ENABLE_INFER_AS is required here, please?
I expected #242 to fix this case.

clang/test/SemaSYCL/usm-params.cpp Outdated Show resolved Hide resolved
@bader
Copy link
Contributor

bader commented Jun 26, 2019

Also, to pass check_pr check, your commit has to be signed (see https://github.com/intel/llvm/blob/intel/CONTRIBUTING.md).

Please, add [SYCL] and [USM] tags to your commit title to make it easier to find in the history.

@jbrodman jbrodman changed the title Initial Support for USM [SYCL][USM] Initial Support for USM Jun 27, 2019
1) Support pointer types in Integration Header
2) Put kernel pointer params in the global addr space
3) Fix CheckSYCLType to support proper recursion through pointers

Signed-off-by: James Brodman <james.brodman@intel.com>
Signed-off-by: James Brodman <james.brodman@intel.com>
Modify test to use generic address space switch in order to pass.

Signed-off-by: James Brodman <james.brodman@intel.com>
Signed-off-by: James Brodman <james.brodman@intel.com>
Signed-off-by: James Brodman <james.brodman@intel.com>
@keryell
Copy link
Contributor

keryell commented Jun 28, 2019

The idea looks good.

bader
bader previously approved these changes Jun 28, 2019
// CHECK-OLD: define{{.*}} spir_kernel {{.*}}void @_ZTSZ4mainE15kernel_function(i32*{{.*}}){{.*}} !dbg [[KERNEL:![0-9]+]] {{.*}}{
// CHECK-NEW: define{{.*}} spir_kernel {{.*}}void @_ZTSZ4mainE15kernel_function(i32 addrspace(4)*{{.*}}){{.*}} !dbg [[KERNEL:![0-9]+]] {{.*}}{

// CHECK: define{{.*}} spir_kernel {{.*}}void @_ZTSZ4mainE15kernel_function(i32 addrspace(1)*{{.*}}){{.*}} !dbg [[KERNEL:![0-9]+]] {{.*}}{
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting.
@asavonic, this looks like a bug with address space handling.

  1. Note, clang emit OpenCL kernel with "generic pointer" argument.
  2. Clang does not emit warnings about captured pointers.

I think both these issues should be fixed for the cases when USM feature is not available.

@bader bader self-assigned this Jun 28, 2019
@bader bader merged commit 6b7fbd9 into intel:sycl Jul 1, 2019
bb-sycl pushed a commit that referenced this pull request Aug 3, 2021
…shifted

Add a comment when there is a shifted value,
    add x9, x0, #291, lsl #12 ; =1191936
but not when the immediate value is unshifted,
    subs x9, x0, #256 ; =256
when the comment adds nothing additional to the reader.

Differential Revision: https://reviews.llvm.org/D107196
bb-sycl pushed a commit that referenced this pull request Apr 15, 2022
…n a loop

When untagging the stack, the compiler may emit a sequence like:
```
        .LBB0_1:
          st2g sp, [sp], #32
          sub x8, x8, #32
          cbnz x8, .LBB0_1
          stg sp, [sp], #16
```
These stack adjustments cannot be described by CFI instructions.

This patch disables merging of SP update with untagging, i.e. makes the
compiler use an additional scratch register (there should be plenty
available at this point as we are in the epilogue) and generate:
```
            mov     x9, sp
            mov     x8, #256
            stg     x9, [x9], #16
    .LBB0_1:
            sub     x8, x8, #32
            st2g    x9, [x9], #32
            cbnz    x8, .LBB0_1
            add     sp, sp, #272
```
Merging is disabled only when we need to generate asynchronous unwind
tables.

Reviewed By: eugenis

Differential Revision: https://reviews.llvm.org/D114548
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.

3 participants