Skip to content

[SYCL] Fix address space for function pointer kernel arguments #5924

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

Merged
merged 1 commit into from
Apr 5, 2022
Merged

[SYCL] Fix address space for function pointer kernel arguments #5924

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
11 changes: 7 additions & 4 deletions clang/lib/Sema/SemaSYCL.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2158,15 +2158,18 @@ class SyclKernelDeclCreator : public SyclKernelFieldHandler {

bool handlePointerType(FieldDecl *FD, QualType FieldTy) final {
// USM allows to use raw pointers instead of buffers/accessors, but these
// pointers point to the specially allocated memory. For pointer fields we
// add a kernel argument with the same type as field but global address
// space, because OpenCL requires it.
// pointers point to the specially allocated memory. For pointer fields,
// except for function pointer fields, we add a kernel argument with the
// same type as field but global address space, because OpenCL requires it.
// Function pointers should have program address space. This is set in
// CodeGen.
QualType PointeeTy = FieldTy->getPointeeType();
Qualifiers Quals = PointeeTy.getQualifiers();
auto AS = Quals.getAddressSpace();
// Leave global_device and global_host address spaces as is to help FPGA
// device in memory allocations
if (AS != LangAS::sycl_global_device && AS != LangAS::sycl_global_host)
if (!PointeeTy->isFunctionType() && AS != LangAS::sycl_global_device &&
AS != LangAS::sycl_global_host)
Quals.setAddressSpace(LangAS::sycl_global);
PointeeTy = SemaRef.getASTContext().getQualifiedType(
PointeeTy.getUnqualifiedType(), Quals);
Expand Down
11 changes: 11 additions & 0 deletions clang/test/CodeGenSYCL/functionptr-addrspace.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,5 +21,16 @@ int main() {
invoke_function(r, &a);
invoke_function(f, &a);
});

// Test function pointer as kernel argument. Function pointers should have program address space i.e. 0.

int (*fptr)();
int *ptr;

// define dso_local spir_kernel void @{{.*}}fake_kernel_2{{.*}}(i32 ()* align 4 %_arg_fptr, i32 addrspace(1)* align 4 %_arg_ptr)
Copy link
Contributor

Choose a reason for hiding this comment

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

Being honest I'm not sure what is the correct address space for function pointers in kernel arguments. It would be great to get feedback from @AlexeySachkov .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just an FYI, since April 2020 (or whenever this commit was pulled down to DPCPP project https://reviews.llvm.org/D77119), the address space has been 0, which is why there was a regression downstream after https://reviews.llvm.org/D119045. If the correct behavior is some other address space, I think it should be done as a follow-up patch (if its not a quick fix). This patch is just fixing the regression

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually I just noticed there are some precommit fails. So maybe this has unintended consequences. I'll take a look.

Copy link
Contributor

Choose a reason for hiding this comment

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

https://github.com/intel/llvm/runs/5747268781?check_suite_focus=true <- this failure is not related and fixed by fa054cc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK thanks for letting me know. There is one more failure on Windows - SYCL :: Printf/mixed-address-space.cpp

I am trying to build a Windows workspace to check what is wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think the failing test is due to this patch. I was unable to reproduce issue on my local system and so I took a closer look at the test - https://github.com/intel/llvm-test-suite/blob/intel/SYCL/Printf/mixed-address-space.cpp. The kernel here isn't capturing anything. So I don't know why this patch would affect it (since the change in this patch only applies to function pointer kernel arguments)

Copy link
Contributor

Choose a reason for hiding this comment

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

since April 2020 (or whenever this commit was pulled down to DPCPP project https://reviews.llvm.org/D77119), the address space has been 0, which is why there was a regression downstream after https://reviews.llvm.org/D119045.

Thanks, this makes me a bit more confident about what the patch does.

Copy link
Contributor

Choose a reason for hiding this comment

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

Being honest I'm not sure what is the correct address space for function pointers in kernel arguments. It would be great to get feedback from @AlexeySachkov .

At SYCL level, we don't plan to have function pointers as kernel arguments

kernel_single_task<class fake_kernel_2>([=]() {
invoke_function(fptr, ptr);
});

return 0;
}