[SYCL] Re-use OpenCL address space attributes for SYCL #1581

Fznamznon · 2020-04-24T11:25:54Z

Today we re-use OpenCL parsed attributes, but have separate SYCL address
space semantic attributes as current implementation of OpenCL semantics
breaks valid C++. This patch enables re-use of OpenCL semantic
attributes by allowing conversions between types qualified with OpenCL
address spaces and type w/o address space qualifiers. Clang compiler
(almost) always adds address space qualifiers in OpenCL mode, so it
should not affect OpenCL mode.

This is rebased and updated version of #1039

Today we re-use OpenCL parsed attributes, but have separate SYCL address space semantic attributes as current implementation of OpenCL semantics breaks valid C++. This patch enables re-use of OpenCL semantic attributes by allowing conversions between types qualified with OpenCL address spaces and type w/o address space qualifiers. Clang compiler (almost) always adds address space qualifiers in OpenCL mode, so it should not affect OpenCL mode. NOTE: this change also disables implicit conversion between the unqualified types and types qualified with `__attribute__((address_space(N)))`, enabled by one of the previous SYCL patches. Signed-off-by: Alexey Bader <[email protected]>

Signed-off-by: Mariya Podchishchaeva <[email protected]>

asavonic · 2020-04-24T11:46:26Z

libdevice/spirv_vars.hpp

@@ -26,9 +26,9 @@ SYCL_EXTERNAL size_t __spirv_LocalInvocationId_z();
 #else // __SYCL_NVPTX__

 typedef size_t size_t_vec __attribute__((ext_vector_type(3)));
-extern "C" const __attribute__((opencl_constant))
+extern "C" const __attribute__((opencl_global))


Can you clarify why this is changed?

Once we re-use OpenCL address space attributes, we re-use diagnostics for them. It seems that OpenCL doesn't allow uninitialized variables in constant address space.

Can we use default address space here?

I think yes. Such change doesn't break my local check-sycl run on CPU device and I don't see any address space requirements in SPIR-V representation in LLVM IR doc .

I was troubled by this as well. I couldn't find a requirement from the OpenCL environment spec.

@bashbaug shouldn't the env spec specify the storage class of builtin variables ?

shouldn't the env spec specify the storage class of builtin variables ?

Yes, I believe it should. Would you mind filing an issue against the SPIR-V Environment spec on https://github.com/khronosgroup/opencl-docs to get this fixed?

Signed-off-by: Mariya Podchishchaeva <[email protected]>

erichkeane

I forgot the justification for the change this is reverting (and now the justification for the revert...) but I remember being properly motivated the last time. The code itself looks fine.

Fznamznon · 2020-04-24T15:22:32Z

@bader , should I fix clang-format fail?
It points to changes disabling OpenCL C++ test.

bader · 2020-04-24T15:25:22Z

Yes formatting issues must be fixed.

rolandschulz · 2020-04-26T01:04:09Z

I'm a bit confused about the plans after this patch. And it seems like the future direction is important to understand whether using the same set of attributes for OCL and SYCL is a good idea.

A couple of questions regarding the following example (for SYCL compiled with -fsycl-device-only -std=c++17, OCL version is https://godbolt.org/z/AeqF4T).

#include <type_traits>

#define __private __attribute__((opencl_private))
#define __local   __attribute__((opencl_local))

//struct S { void f() __private; }; //should be OK?                                                                                                                                                                

void t() {
    //__private int i1; //should be OK?                                                                                                                                                                            
    int i2;
    static_assert(std::is_same_v<decltype(&i2), int*>); //intentional difference                                                                                                                                   
    __private int* p;
    __local int* l;
    p-l; //should be error "non-overlapping address spaces"?                                                                                                                                                       
}

Why does it error where OpenCL for C++ doesn't and not error in cases where OpenCL does? To me the behavior of OpenCL makes much more sense in all three cases. Are these differences intentional? And if so why? Or are are they considered bugs? If so should those be fixed as part of this change or in follow up changes?

On the other hand there are intentional differences like the one tested with the static_assert. How do we make sure that it'll be easy to maintain those difference going forward if we reuse the same attribute?

Fznamznon · 2020-04-27T09:49:54Z

Why does it error where OpenCL for C++ doesn't and not error in cases where OpenCL does? To me the behavior of OpenCL makes much more sense in all three cases. Are these differences intentional? And if so why? Or are are they considered bugs? If so should those be fixed as part of this change or in follow up changes?

I can see errors in OpenCL for C++ (https://godbolt.org/z/3aaoVQ), but there is no errors in SYCL. It happens because the corresponding diagnostic is wrapped with if (OpenCL) and Sema just doesn't run them for SYCL (see

llvm/clang/lib/Sema/SemaExpr.cpp

Line 9705 in cece82e

if (S.getLangOpts().OpenCL && isLHSPointer && isRHSPointer) {

). I think we just didn't pay attention to re-using all the diagnostic for OpenCL address space attributes and I'm not sure that missing those diagnostics in SYCL is a problem at all, because SYCL language doesn't define those attributes for user.

On the other hand there are intentional differences like the one tested with the static_assert. How do we make sure that it'll be easy to maintain those difference going forward if we reuse the same attribute?

Why it can be a problem? Do you have any examples/arguments?

Naghasan · 2020-04-27T10:55:42Z

On the other hand there are intentional differences like the one tested with the static_assert. How do we make sure that it'll be easy to maintain those difference going forward if we reuse the same attribute?

Why it can be a problem? Do you have any examples/arguments?

This introduce a semantic difference between SYCL and OpenCL that needs to be maintained.
So is it realistic to maintain, on a long term, this semantic difference ? Might be good to understand what make this static_assert fails. Looking at the ast dump, i2's type is __private int and int* implicitly gets promoted to __generic int*, so I'm wondering if this is fully contained in SemaType.

On the other hand, is it realistic (especially with the upstreaming objective) to clone the whole logic just because there is a few cases where the logic diverges ? Cloning the logic has plenty of other consequences, and I would personally be much more concerned about maintaining a fully cloned logic rather than maintaining a few and controlled differences.

Fznamznon · 2020-04-27T13:17:51Z

On the other hand there are intentional differences like the one tested with the static_assert. How do we make sure that it'll be easy to maintain those difference going forward if we reuse the same attribute?

Why it can be a problem? Do you have any examples/arguments?

This introduce a semantic difference between SYCL and OpenCL that needs to be maintained.
So is it realistic to maintain, on a long term, this semantic difference ? Might be good to understand what make this static_assert fails. Looking at the ast dump, i2's type is __private int and int* implicitly gets promoted to __generic int*, so I'm wondering if this is fully contained in SemaType.

I think this difference happens because in OpenCL the compiler is intended to add address space qualifiers to all (at least almost all) pointers. AFAIK it is done during parsing, there is a good explanation #1039 (comment). We don't do this for SYCL because it can lead to problems in C++ code (I think @erichkeane can explain it much more better than me).

rolandschulz · 2020-04-27T13:56:19Z

On the other hand, is it realistic (especially with the upstreaming objective) to clone the whole logic just because there is a few cases where the logic diverges ? Cloning the logic has plenty of other consequences, and I would personally be much more concerned about maintaining a fully cloned logic rather than maintaining a few and controlled differences.

Makes sense.

rolandschulz · 2020-04-27T14:06:03Z

I think we just didn't pay attention to re-using all the diagnostic for OpenCL address space attributes and I'm not sure that missing those diagnostics in SYCL is a problem at all, because SYCL language doesn't define those attributes for user.

SYCL doesn't define the attributes but it defines multi_ptr::pointer_ptr for the user to use them. Therefore we should fix the any unintentional differences (not just the missing error but also the two cases where it errors when it shouldn't). But it doesn't have to happen as part of this patch.

tadeaustria · 2020-04-28T10:52:36Z

since the merge of this I get following errors on compiling a simple sycl program
/CL/__spirv/spirv_vars.hpp:167:10: error: variable in constant address space must be initialized uint32_t __spirv_BuiltInSubgroupLocalInvocationId;

bader · 2020-04-28T11:02:49Z

since the merge of this I get following errors on compiling a simple sycl program
/CL/__spirv/spirv_vars.hpp:167:10: error: variable in constant address space must be initialized uint32_t __spirv_BuiltInSubgroupLocalInvocationId;

This is strange...
This variable is not in constant address space https://github.com/intel/llvm/blob/sycl/sycl/include/CL/__spirv/spirv_vars.hpp#L136.
I suspect that you might be using new compiler + old headers.
@tadeaustria, could you make sure that the spirv_vars.hpp header is up-to-date, please?

tadeaustria · 2020-04-28T12:12:29Z

This is strange...
This variable is not in constant address space https://github.com/intel/llvm/blob/sycl/sycl/include/CL/__spirv/spirv_vars.hpp#L136.
I suspect that you might be using new compiler + old headers.
@tadeaustria, could you make sure that the spirv_vars.hpp header is up-to-date, please?

Ah, yes, thanks for the hint! the compiler used outdated headers. Works fine after updating them

bader and others added 5 commits April 23, 2020 17:16

Fix devicelib build

c486c16

Signed-off-by: Mariya Podchishchaeva <[email protected]>

Fix clang tests

dbc9b7e

Signed-off-by: Mariya Podchishchaeva <[email protected]>

Use default address space for for SPIR-V builtin vars

d7b4d48

Signed-off-by: Mariya Podchishchaeva <[email protected]>

Fix LIT tests for CUDA

730168a

Signed-off-by: Mariya Podchishchaeva <[email protected]>

Fznamznon requested review from asavonic, erichkeane, vzakhari and a team as code owners April 24, 2020 11:25

Fznamznon requested a review from sergey-semenov April 24, 2020 11:25

asavonic reviewed Apr 24, 2020

View reviewed changes

Apply clang-format

8b6519f

Signed-off-by: Mariya Podchishchaeva <[email protected]>

Naghasan previously approved these changes Apr 24, 2020

View reviewed changes

erichkeane previously approved these changes Apr 24, 2020

View reviewed changes

Fznamznon dismissed stale reviews from erichkeane and Naghasan via 8b6519f April 24, 2020 15:31

bader requested review from erichkeane and asavonic April 24, 2020 17:58

erichkeane approved these changes Apr 24, 2020

View reviewed changes

vzakhari approved these changes Apr 25, 2020

View reviewed changes

This was referenced Apr 25, 2020

[SYCL] Re-use OpenCL address space attributes for SYCL #1039

Closed

WIP: [SYCL] Implement SYCL address space attributes handling #968

Closed

sergey-semenov approved these changes Apr 27, 2020

View reviewed changes

Naghasan mentioned this pull request Apr 27, 2020

SPIR-V OpenCL environment spec does not describe Builtin decoration KhronosGroup/OpenCL-Docs#269

Closed

Naghasan mentioned this pull request Apr 27, 2020

[SPIR-V] Correct/improve declaration of some SPIR-V builtins #1519

Merged

bader merged commit ac94b1e into intel:sycl Apr 27, 2020

rolandschulz mentioned this pull request Apr 27, 2020

Unintended different behavior between SYCL and OpenCL attributes #1596

Open

[SYCL] Re-use OpenCL address space attributes for SYCL #1581

[SYCL] Re-use OpenCL address space attributes for SYCL #1581

Uh oh!

Conversation

Fznamznon commented Apr 24, 2020

Uh oh!

asavonic Apr 24, 2020

Choose a reason for hiding this comment

Uh oh!

Fznamznon Apr 24, 2020

Choose a reason for hiding this comment

Uh oh!

asavonic Apr 24, 2020

Choose a reason for hiding this comment

Uh oh!

Fznamznon Apr 24, 2020

Choose a reason for hiding this comment

Uh oh!

Naghasan Apr 24, 2020

Choose a reason for hiding this comment

Uh oh!

bashbaug Apr 24, 2020

Choose a reason for hiding this comment

Uh oh!

erichkeane left a comment

Choose a reason for hiding this comment

Uh oh!

Fznamznon commented Apr 24, 2020

Uh oh!

bader commented Apr 24, 2020

Uh oh!

rolandschulz commented Apr 26, 2020

Uh oh!

Fznamznon commented Apr 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Naghasan commented Apr 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Fznamznon commented Apr 27, 2020

Uh oh!

rolandschulz commented Apr 27, 2020

Uh oh!

rolandschulz commented Apr 27, 2020

Uh oh!

tadeaustria commented Apr 28, 2020

Uh oh!

bader commented Apr 28, 2020

Uh oh!

tadeaustria commented Apr 28, 2020

Uh oh!

Uh oh!

Fznamznon commented Apr 27, 2020 •

edited

Loading

Naghasan commented Apr 27, 2020 •

edited

Loading