Skip to content

[SYCL][HIP][libclc] Fix wrong address spaces for HIP #4966

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
Jan 11, 2022
Merged

[SYCL][HIP][libclc] Fix wrong address spaces for HIP #4966

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
2 changes: 2 additions & 0 deletions clang/lib/Basic/Targets/AMDGPU.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -328,6 +328,8 @@ class LLVM_LIBRARY_VISIBILITY AMDGPUTargetInfo final : public TargetInfo {
case OCLTK_Queue:
case OCLTK_ReserveID:
return LangAS::opencl_global;
case OCLTK_Event:
return LangAS::opencl_private;

default:
return TargetInfo::getOpenCLTypeAddrSpace(TK);
Expand Down
1 change: 1 addition & 0 deletions libclc/amdgcn-amdhsa/libspirv/SOURCES
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,3 +15,4 @@ workitem/get_sub_group_id.cl
workitem/get_sub_group_local_id.cl
workitem/get_sub_group_size.cl
misc/sub_group_shuffle.cl
async/wait_group_events.cl
17 changes: 17 additions & 0 deletions libclc/amdgcn-amdhsa/libspirv/async/wait_group_events.cl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
//===----------------------------------------------------------------------===//
//
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
// See https://llvm.org/LICENSE.txt for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
//
//===----------------------------------------------------------------------===//

#include <spirv/spirv.h>

_CLC_DEF void _Z23__spirv_GroupWaitEventsjiP9ocl_event(unsigned int scope,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
_CLC_DEF void _Z23__spirv_GroupWaitEventsjiP9ocl_event(unsigned int scope,
_CLC_DEF _CLC_OVERLOAD void __spirv_GroupWaitEvents(unsigned int scope,

it is also missing the overload for private

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 tried adding that, but I can't get it to work (linker is not finding this).

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/blob/sycl/clang/lib/AST/ItaniumMangle.cpp#L2560 is causing different mangling.

In SYCL mode, Default will be 0, so no UAS0 will be added, by in CL mode, Default is 5 (private) causing the addition of UAS0.

This could either be recovered by the remangler (a bit dirty but more scalable) or handled by manual mangling (not great).

int num_events,
event_t __attribute__((address_space(0)))* event_list) {
Copy link
Contributor

@bader bader Nov 16, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I think using __private is better here.
OTOH, I don't fully understand the problem. If we compile this code with -cl-std=CL2.0, event_list will point to generic address space.
What exactly is the problem here?

Copy link
Contributor

Choose a reason for hiding this comment

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

0 is generic here (5 is private)

If we compile this code with -cl-std=CL2.0

there is a need for selective enabling of 2.0. The lib is tailored for 1.2 so that will probably not turned as expected (you suddenly make pointers to private pointers to generic).

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh... wait. Are we calling OpenCL 1.2 built-in here? Shouldn't we apply private address space attribute on SYCL side in this case and explicitly cast to correct AS?

Copy link
Contributor

Choose a reason for hiding this comment

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

Well there is an overload for generic. Never mattered for PTX because the target address space for private and generic overlap.

The runtime header could be patch to call the privare overload (it will always be a pointer to private AFAIK).

But TBH, there is an issue in the device event design as the stored pointer is a dangling one #4959

Copy link
Contributor

Choose a reason for hiding this comment

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

The runtime header could be patch to call the privare overload (it will always be a pointer to private AFAIK).

So, which way do you think is better to handle this issue: update the runtime to call correct overload overload or apply this patch?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Naghasan I think the question above was meant for you.

Copy link
Contributor

Choose a reason for hiding this comment

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

oups sorry

So, which way do you think is better to handle this issue: update the runtime to call correct overload overload or apply this patch?

There is a need for a broader course of action regarding generic address space handling here, so as the patch is ready, I would say ship it. Once the generic is better handled, this will go away.

__spirv_ControlBarrier(scope, Workgroup, SequentiallyConsistent);
}