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

t4c1 · 2021-11-16T14:11:53Z

Fixes wrong address spaces for event_t. It should be private, but in SYCL it was defaulting to generic. On the other hand in libclc pointer to event_t was private, while it should be generic. (We don't see similar issues on NVPTX, as there private address space is in fact implemented as generic in IR).

Change to test suite: intel/llvm-test-suite#570

Naghasan · 2021-11-16T14:21:15Z

libclc/amdgcn-amdhsa/libspirv/async/wait_group_events.cl

+
+#include <spirv/spirv.h>
+
+_CLC_DEF void _Z23__spirv_GroupWaitEventsjiP9ocl_event(unsigned int scope,


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

I tried adding that, but I can't get it to work (linker is not finding this).

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).

bader · 2021-11-16T15:31:54Z

libclc/amdgcn-amdhsa/libspirv/async/wait_group_events.cl

+
+_CLC_DEF void _Z23__spirv_GroupWaitEventsjiP9ocl_event(unsigned int scope,
+                                                    int num_events,
+                                                    event_t __attribute__((address_space(0)))* event_list) {


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?

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).

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?

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

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?

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

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.

smanna12

FE changes LGTM

…ess spaces (#570) Remove xfail from tests fixed by: intel/llvm#4966

bader · 2022-01-11T12:22:52Z

@t4c1, this patch breaks check-libclc https://github.com/intel/llvm/runs/4773520788?check_suite_focus=true. Please, fix as soon as possible.

t4c1 · 2022-01-11T14:26:18Z

I looked into this, but I am not even sure what that test is supposed to be checking. It seems completely broken - for both CUDA and HIP backends it is only passing because the code does not compile and the test only checks with FileCheck and CHECK-NOT.

bader · 2022-01-11T15:13:31Z

I'm reverting this patch to fix pre-commit checks in CI blocking other PRs.
I suggest talking to @Naghasan. Probably you just need to re-generate the checks with gen-libclc-test.py.

This reverts commit 22532c2.

…#5284) This reverts commit 22532c2. This patch breaks check-libclc tests.

…t_t address spaces (#570)" (#724) This reverts commit 367e10e. Reverting due to intel/llvm#4966 revert.

…ntel#4966)" (intel#5284)" This reverts commit 29047bc.

…5293) Reverts #5284 , reintroducing the reverted #4966 and adds fixes the failing libclc test.

…ess spaces (intel/llvm-test-suite#570) Remove xfail from tests fixed by: intel#4966

…t_t address spaces (intel/llvm-test-suite#570)" (intel/llvm-test-suite#724) This reverts commit d2ed0fc. Reverting due to intel#4966 revert.

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

0e36016

t4c1 requested review from bader, elizabethandrews, premanandrao and smanna12 as code owners November 16, 2021 14:11

t4c1 mentioned this pull request Nov 16, 2021

[SYCL][HIP] Remove xfail from fails due to wrong event_t address space intel/llvm-test-suite#570

Merged

Naghasan reviewed Nov 16, 2021

View reviewed changes

bader reviewed Nov 16, 2021

View reviewed changes

bader requested a review from Naghasan November 30, 2021 13:11

bader approved these changes Jan 10, 2022

View reviewed changes

smanna12 approved these changes Jan 10, 2022

View reviewed changes

bader merged commit 22532c2 into intel:sycl Jan 11, 2022

bader pushed a commit to intel/llvm-test-suite that referenced this pull request Jan 11, 2022

[SYCL][HIP] remove xfail form tests failing due to wrong event_t addr…

367e10e

…ess spaces (#570) Remove xfail from tests fixed by: intel/llvm#4966

bader added a commit that referenced this pull request Jan 11, 2022

Revert "[SYCL][HIP][libclc] Fix wrong address spaces for HIP (#4966)"

c5e7291

This reverts commit 22532c2.

bader mentioned this pull request Jan 11, 2022

Revert "[SYCL][HIP][libclc] Fix wrong address spaces for HIP" #5284

Merged

bader added a commit that referenced this pull request Jan 11, 2022

Revert "[SYCL][HIP][libclc] Fix wrong address spaces for HIP (#4966)" (…

29047bc

…#5284) This reverts commit 22532c2. This patch breaks check-libclc tests.

bader mentioned this pull request Jan 11, 2022

Revert "[SYCL][HIP] Remove xfail from fails due to wrong event_t address space" intel/llvm-test-suite#724

Merged

bader added a commit to intel/llvm-test-suite that referenced this pull request Jan 11, 2022

Revert "[SYCL][HIP] remove xfail form tests failing due to wrong even…

0b6fc11

…t_t address spaces (#570)" (#724) This reverts commit 367e10e. Reverting due to intel/llvm#4966 revert.

t4c1 added a commit to t4c1/llvm that referenced this pull request Jan 12, 2022

Revert "Revert "[SYCL][HIP][libclc] Fix wrong address spaces for HIP (i…

287bd90

…ntel#4966)" (intel#5284)" This reverts commit 29047bc.

t4c1 mentioned this pull request Jan 12, 2022

[SYCL][HIP][libclc] Revert revert "Fix wrong address spaces for HIP" #5293

Merged

bader pushed a commit that referenced this pull request Jan 13, 2022

[SYCL][HIP][libclc] Revert revert "Fix wrong address spaces for HIP" (#…

03970cc

…5293) Reverts #5284 , reintroducing the reverted #4966 and adds fixes the failing libclc test.

t4c1 deleted the group_async_copy_aperture branch March 15, 2022 08:51

aelovikov-intel pushed a commit to aelovikov-intel/llvm that referenced this pull request Mar 27, 2023

[SYCL][HIP] remove xfail form tests failing due to wrong event_t addr…

d2ed0fc

…ess spaces (intel/llvm-test-suite#570) Remove xfail from tests fixed by: intel#4966

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

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

Uh oh!

t4c1 commented Nov 16, 2021 •

edited

Loading

Uh oh!

Naghasan Nov 16, 2021

Uh oh!

t4c1 Nov 16, 2021

Uh oh!

Naghasan Nov 16, 2021

Uh oh!

bader Nov 16, 2021 •

edited

Loading

Uh oh!

Naghasan Nov 16, 2021

Uh oh!

bader Nov 16, 2021

Uh oh!

Naghasan Nov 17, 2021

Uh oh!

bader Nov 30, 2021

Uh oh!

t4c1 Jan 4, 2022

Uh oh!

Naghasan Jan 4, 2022

Uh oh!

smanna12 left a comment

Uh oh!

bader commented Jan 11, 2022

Uh oh!

t4c1 commented Jan 11, 2022

Uh oh!

bader commented Jan 11, 2022

Uh oh!

Uh oh!


		#include <spirv/spirv.h>

		_CLC_DEF void _Z23__spirv_GroupWaitEventsjiP9ocl_event(unsigned int scope,

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

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

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

Uh oh!

Conversation

t4c1 commented Nov 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bader Nov 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smanna12 left a comment

Choose a reason for hiding this comment

Uh oh!

bader commented Jan 11, 2022

Uh oh!

t4c1 commented Jan 11, 2022

Uh oh!

bader commented Jan 11, 2022

Uh oh!

Uh oh!

t4c1 commented Nov 16, 2021 •

edited

Loading

bader Nov 16, 2021 •

edited

Loading