Page MenuHomePhabricator

[SYCL] Implement SYCL address space attributes handling
Needs ReviewPublic

Authored by bader on Oct 21 2020, 1:34 PM.

Details

Summary

Default address space (applies when no explicit address space was
specified) maps to generic (4) address space.

Added SYCL named address spaces sycl_global, sycl_local and
sycl_private defined as sub-sets of the default address space.

Static variables without address space now reside in global address
space when compile for SPIR target, unless they have an explicit address
space qualifier in source code.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Anastasia added inline comments.Wed, Apr 7, 10:54 AM
clang/lib/Basic/Targets/SPIR.h
127

Should the following code be legal? Perhaps you can replace this with a spec reference instead?

132

This only makes sense for SYCL so let's add a guard.

clang/lib/CodeGen/TargetInfo.cpp
10047

Is this change related to address spaces?

clang/test/CodeGenSYCL/address-space-cond-op.cpp
1 ↗(On Diff #333247)

Is this comment relevant?

39 ↗(On Diff #333247)

This code doesn't seem to be relevant to the testing.

clang/test/CodeGenSYCL/address-space-of-returns.cpp
8 ↗(On Diff #333247)

Are you testing address space segment binding here? Could you extend to other use cases too?

clang/test/CodeGenSYCL/address-space-parameter-conversions.cpp
1 ↗(On Diff #333247)

How about adding explicit conversion here too and then renaming into something like address-space-conversions.cpp since the testing is not specific to a parameter.

80 ↗(On Diff #333247)

What functionality in the patch does this test?

99 ↗(On Diff #333247)

The same functionality seems to be tested above.

124 ↗(On Diff #333247)

Does this test anything from the patch?

clang/test/CodeGenSYCL/address-spaces-struct.cpp
1 ↗(On Diff #333247)

What do you test in this file?

clang/test/CodeGenSYCL/address-spaces.cpp
1 ↗(On Diff #333247)

Does this test also check conversions? Could this be merged with another conversions test if there is anything missing there?

clang/test/SemaSYCL/address-space-parameter-conversions.cpp
1 ↗(On Diff #333247)

I think this should also be renamed to address-space-conversions.cpp

You shouldn't need to add -x c++ as it is default for cpp files.

You should add testing of diagnostics for disallowed conversions too.

25 ↗(On Diff #333247)

Btw you don't seem to be testing the reverse conversion i.e. from named to Default.

47 ↗(On Diff #333247)

Why?

53 ↗(On Diff #333247)

This doesn't belong to conversions technically.

bader updated this revision to Diff 336543.Fri, Apr 9, 12:24 PM
bader marked 32 inline comments as done.

Applied code review suggestions.

Rebased on ToT and updated commit message.

clang/include/clang/AST/Type.h
493

Ok if you allow implicit conversions both ways then this condition should be extended to also contain all named address spaces in A and Default in B. But actually, could you simplify by checking that you have Default on either side, so something like

(A == LangAS::Default || B == LangAS::Default)

?

According to the comment above isAddressSpaceSupersetOf function definition.

/// Returns true if address space A is equal to or a superset of B.

(A == LangAS::Default || B == LangAS::Default) <- this change makes Default address space a superset of all address spaces including OpenCL, which we were trying to avoid with adding SYCL address spaces. Another problem with this code is that make Default a sub-set of named address spaces (like sycl_local), which is not right.
If I understand it correctly defining "isSupersSetOf" relation is enough for the rest of framework to enable conversions. Am I right?

clang/lib/AST/ItaniumMangle.cpp
2465

Any reason not to use OpenCL mangling? If you do then you might be able to link against libraries compiled for OpenCL. Also you will get more stable naming i.e. it would not differ from target to target.

I'm not sure I understand your suggestion. Could you elaborate on "OpenCL mangling", please?

Let me clarify the problem this change addresses. The test case covering it is located in clang/test/CodeGenSYCL/address-space-parameter-conversions.cpp lines 86-91.

template <typename T>
void tmpl(T t) {}

int *NoAS;
__attribute__((opencl_private)) int *PRIV;

tmpl(PRIV);
// CHECK-DAG: [[PRIV_LOAD5:%[a-zA-Z0-9]+]] = load i32*, i32* addrspace(4)* [[PRIV]].ascast
// CHECK-DAG: call spir_func void [[PRIV_TMPL:@[a-zA-Z0-9_]+]](i32* [[PRIV_LOAD5]])
tmpl(NoAS);
// CHECK-DAG: [[NoAS_LOAD5:%[a-zA-Z0-9]+]] = load i32 addrspace(4)*, i32 addrspace(4)* addrspace(4)* [[NoAS]].ascast
// CHECK-DAG: call spir_func void [[GEN_TMPL:@[a-zA-Z0-9_]+]](i32 addrspace(4)* [[NoAS_LOAD5]])

Clang has separate code paths for mangling types w/ and w/o address space attributes (i.e. using Default address space).

Address space is not mangled if there is no AS attribute (Default) or if address space attribute is maps to 0 target address space. SPIR target maps *_private address space to 0, which causes name conflict for the example above.

This change for SYCL compiler enables mangling for non-default address space attributes regardless of their mapping to target address space.

clang/lib/Basic/Targets/AMDGPU.cpp
74

I can't find an example of how to do this.
CUDA address spaces use valid values and I wasn't able to find similar comments.

Where do you think we can put a comment?

clang/lib/Basic/Targets/SPIR.h
36

I've set 0 for SYCL values.

77

Good suggestion. I see that AMDGPU target does the same trick with overriding ajust method.
I also added back SPIR target assertions for unknow environment and OS components to clang/lib/Basic/Targets.cpp.

132

This only makes sense for SYCL so let's add a guard.

Unfortunately, I don't know what can be used as a guard here. TargetInfo doesn't provide access to language options.
Any suggestions?

clang/lib/CodeGen/TargetInfo.cpp
10047

I'll move this change to a separate patch.

10076

I think this comment should be improved because OpenCL and CUDA are not really address space agnostic. Should this check also contain C or C++?

I think the assert says that OpenCL and CUDA are not address space agnostic languages.
Tagging @yaxunl as an author of the check I just borrowed from AMDGPU target.

10077

This code assigns target address space "global variables w/o address space attribute".
SYCL says it's "implementation defined" (from https://www.khronos.org/registry/SYCL/specs/sycl-2020/html/sycl-2020.html#subsec:commonAddressSpace):

Namespace scope
If the type is const, the address space the declaration is assigned to is implementation-defined. If the target of the SYCL backend can represent the generic address space, then the assigned address space must be compatible with the generic address space.
Namespace scope non-const declarations cannot be used within a kernel, as restricted in Section 5.4. This means that non-const global variables cannot be accessed by any device kernel or code called by the device kernel.

I added clarification that SPIR target allocates global variables in global address space to https://reviews.llvm.org/D99488 (see line #248).

@rjmccall, mentioned in the mailing list discussion that this callbacks were developed for compiling C++ to AMDGPU target, so this not necessary designed only for SYCL, but it works for SYCL as well.

clang/test/CodeGenSYCL/address-space-cond-op.cpp
1 ↗(On Diff #333247)

Is this comment relevant?

I think so. It's placed by the script and useful for test maintenance.

39 ↗(On Diff #333247)

Probably not for this patch, but the next patch (https://reviews.llvm.org/D71016) makes it necessary to emit LLVM IR for this test. Is it okay to leave it as is?

clang/test/CodeGenSYCL/address-space-of-returns.cpp
8 ↗(On Diff #333247)

Are you testing address space segment binding here? Could you extend to other use cases too?

This test checks that returned address space is generic and address space is correctly casted for character, array, reference and aggregate types.

Could you clarify how exactly do you want this test to be extended, please? I think I'm not getting it.

clang/test/CodeGenSYCL/address-space-parameter-conversions.cpp
80 ↗(On Diff #333247)

What functionality in the patch does this test?

As I mentioned in the comment for clang/lib/AST/ItaniumMangle.cpp, there was a problem with instantiating templates for parameters which differs only by address space attribute. Lines 79-91 cover mangling changes.

99 ↗(On Diff #333247)

Removed.

124 ↗(On Diff #333247)

Does this test anything from the patch?

It's the same case as for address-space-cond-op.cpp test. This function defines an entry point for the device code. If it's not called, SYCL device compiler won't emit any LLVM IR output. I uploaded the final version of the test, which assumes that SYCL device compiler code generation part is in place. I would prefer to keep it instead of adding it back with my next patch. Is this okay?

clang/test/CodeGenSYCL/address-spaces-struct.cpp
1 ↗(On Diff #333247)

What do you test in this file?

This test was added to cover a bug in CodeGen with incompatible address spaces between pointers to a struct and its members.
As we replaced CodeGen changes with SPIR target callbacks I think it's not needed anymore. I removed this test.

BTW, the same applies to clang/test/CodeGenSYCL/address-space-cond-op.cpp. Should we remove it as well?

clang/test/CodeGenSYCL/address-spaces.cpp
1 ↗(On Diff #333247)

Does this test also check conversions? Could this be merged with another conversions test if there is anything missing there?

I renamed this to address-space-deduction.cpp and address-space-parameter-conversions.cpp to address-space-conversions.cpp.

clang/test/SemaSYCL/address-space-parameter-conversions.cpp
25 ↗(On Diff #333247)

Btw you don't seem to be testing the reverse conversion i.e. from named to Default.

Don't bar2(*GLOB);, bar2(*LOC);, bar2(*PRIV); cover this conversion?

Anastasia added inline comments.Mon, Apr 12, 7:56 AM
clang/include/clang/AST/Type.h
493

(A == LangAS::Default || B == LangAS::Default) <- this change makes Default address space a superset of all address spaces including OpenCL.

I see, yes this will break pretty much everything unless we guard by SYCL mode. But I don't think it is good to go this route though.

Another problem with this code is that make Default a sub-set of named address spaces (like sycl_local), which is not right.

Well, if you need implicit conversions to work both ways as you have written in the documentation then you don't really have a true super-/subsets between the named address spaces and the default one. They appear to be equivalent.

SYCL mode enables both explicit and implicit conversion to/from the default address space from/to
the address space-attributed type.

So do you actually need something like this to work?

int * genptr = ...;
__private int * privptr = genptr:
clang/lib/AST/ItaniumMangle.cpp
2465

It's just that all language address spaces are mangled with the source spelling in Italium ABI right now, if you check the else statement. I don't think it is part of the official spec yet but it might be better to stick to the same pattern if possible.

clang/lib/Basic/Targets/AMDGPU.cpp
74

I think you could add a comment right here. I bet you can use any values i.e. 0? They are not expected to be meaningful. It would be good if we could add an asset somewhere but perhaps it might be hard to find a good place...

clang/lib/Basic/Targets/SPIR.h
36

Great! Let's add a comment here that we use dummy values because the map can't be used for this language mode.

46

Ok let's set the OpenCL address spaces to 0 and add a comment that they can't be used with this map.

131

Ok, do you still plan to add a FIXME as mentioned previously explaining why we need to reset the map here?

clang/lib/CodeGen/TargetInfo.cpp
10077

After all what objects are allowed to bind to non-default address space here is defined in SYCL spec even if the exact address spaces are not defined so it is not completely a target-specific behavior.

My understanding of the API you are extending (judging from its use) is that it allows you to extend the language sematics with some target-specific setup. I.e. you could add extra address spaces to C++ or OpenCL or any other language. But here you are setting the language address spaces instead that are mapped to the target at some point implicitly.

It seems like this change better fits to CodeGenModule::GetGlobalVarAddressSpace that already contains very similar logic?

Otherwise, it makes more sense to use target address spaces directly instead of SYCL language address spaces. But either way, we should guard it by SYCL mode somehow as we have not established this as a universal logic for SPIR.

clang/test/CodeGenSYCL/address-space-cond-op.cpp
8 ↗(On Diff #336543)

I am trying to understand what specifically you are testing from the patch and whether you need this whole IR output to be checked?

It helps for reviewing and maintenance purposes to minimize the IR patterns as much as possible.

39 ↗(On Diff #333247)

I would prefer if we minimize the amount of noise as much as possible. This patch has enough functionality so I don't think adding extra helps in any way.

The testing should generally encapsulate what is relevant. Otherwise, I don't know how to assess it without reviewing other patches too.

clang/test/CodeGenSYCL/address-space-conversions.cpp
13

Should you check that the function name has expected mangling?

clang/test/CodeGenSYCL/address-space-of-returns.cpp
8 ↗(On Diff #333247)

It seems to me that you are just testing address space inference here similar to address-space-deduction.cpp? I don't see anything specific to a return type in either the test or your patch?

clang/test/CodeGenSYCL/convergent.cpp
2 ↗(On Diff #336543)

Is this change related? I thought we are not adding the environment component after all...

Anastasia added inline comments.Mon, Apr 12, 8:05 AM
clang/test/CodeGenSYCL/address-space-parameter-conversions.cpp
80 ↗(On Diff #333247)

You are not testing the mangling though?

124 ↗(On Diff #333247)

If you are not testing any logic from this patch let's leave it out because it is hard to navigate in the large review.

clang/test/CodeGenSYCL/address-spaces-struct.cpp
1 ↗(On Diff #333247)

Yeah if it is not testing anything in your patch then it's better to remove it.

clang/test/SemaSYCL/address-space-parameter-conversions.cpp
25 ↗(On Diff #333247)

Sorry I mean from Default to named AS.

bader marked an inline comment as done.Mon, Apr 12, 10:57 AM
bader added inline comments.
clang/include/clang/AST/Type.h
488

BTW, we need enable global_device and global_host attributes from https://reviews.llvm.org/D82174 for SYCL USM feature. I have following question regarding this: should I create a follow-up patch or we can enable all attributes for SYCL at once?

clang/test/CodeGenSYCL/convergent.cpp
2 ↗(On Diff #336543)

Is this change related? I thought we are not adding the environment component after all...

While I was removing -sycldevice environment component from the patch, I noticed that one of the committed tests already uses it.
https://github.com/llvm/llvm-project/blob/main/clang/test/CodeGenSYCL/convergent.cpp#L2

Do you want to me to create a separate review request for this change?

Anastasia added inline comments.Tue, Apr 13, 6:57 AM
clang/include/clang/AST/Type.h
488

It seems like they would just be extending the existing functionality and not redesigning what we do in this patch?

If that's the case let's keep it in a separate patch, but feel free to upload it even now.

clang/lib/Basic/Targets/SPIR.h
131

Btw I was just thinking of another alternative here.

What do you think about just setting a value in Default AS entry then we wouldn't need any extra map at all in this case? So something like:

AddrSpaceMap[LangAS::Default] = AddrSpaceMap[LangAS::opencl_generic];

with a good explanation in FIXME? :)

clang/test/CodeGenSYCL/convergent.cpp
2 ↗(On Diff #336543)

I see, this looks trivial enough test clean up. You could just commit it without any review perhaps?

bader updated this revision to Diff 337180.Tue, Apr 13, 9:02 AM
bader marked 16 inline comments as done.

Applied more code review suggestions.

Rebased on ToT.

bader added inline comments.Tue, Apr 13, 9:03 AM
clang/include/clang/AST/Type.h
493

I looked though the code base and I see that explicit cast is used when raw pointer is casted to address space annotated type. I think we can always use explicit cast from Default to named address space instead of implicit cast. It might be even useful to avoid unintended implicit casts causing UB.
@keryell, @Naghasan, what do you think if we update https://reviews.llvm.org/D99488 to disallow implicit casts from Default to named address space? I think it should be okay considering that current implementation doesn't use this type of casts (and I can't come up with a use case for it).

Meanwhile I've added checks for that to clang/test/SemaSYCL/address-space-conversions.cpp.

clang/lib/AST/ItaniumMangle.cpp
2465

It's just that all language address spaces are mangled with the source spelling in Italium ABI right now, if you check the else statement. I don't think it is part of the official spec yet but it might be better to stick to the same pattern if possible.

I would really love to avoid changes to the mangler (e.g. to be able to link binaries produced by different front-end like SYCL/OpenCL/CUDA), but I don't know the better way to address the issue
Sorry, I don't get what do you suggest here. Could you clarify what exactly I should change, please?

clang/lib/Basic/Targets/AMDGPU.cpp
74

I think you could add a comment right here. I bet you can use any values i.e. 0? They are not expected to be meaningful. It would be good if we could add an asset somewhere but perhaps it might be hard to find a good place...

I've set Generic value (i.e. 0) and added a comment.

clang/lib/Basic/Targets/SPIR.h
131

Btw I was just thinking of another alternative here.

What do you think about just setting a value in Default AS entry then we wouldn't need any extra map at all in this case? So something like:

AddrSpaceMap[LangAS::Default] = AddrSpaceMap[LangAS::opencl_generic];

with a good explanation in FIXME? :)

I think this won't work because AddrSpaceMap is a constant.
Regarding FIXME, could you point where it was previously mentioned, please? I think it might miss it. I can add a link to the SYCL spec - https://www.khronos.org/registry/SYCL/specs/sycl-2020/html/sycl-2020.html#subsec:genericAddressSpace. Will it be sufficient?

clang/lib/CodeGen/TargetInfo.cpp
10077

It seems like this change better fits to CodeGenModule::GetGlobalVarAddressSpace that already contains very similar logic?

This was the original implementation (see https://reviews.llvm.org/D89909?id=299795), but @rjmccall suggested to use this callback instead.
Both ways work for me, but the implementation proposed by John is easier to maintain.

Otherwise, it makes more sense to use target address spaces directly instead of SYCL language address spaces. But either way, we should guard it by SYCL mode somehow as we have not established this as a universal logic for SPIR.

I've updated the code to use target address space. I also added an assertion for SYCL language mode, although I think SPIR doesn't support global variables in address spaces other than global or constant regardless of the language mode, so I think the logic is universal.

clang/test/CodeGenSYCL/address-space-cond-op.cpp
8 ↗(On Diff #336543)

I am trying to understand what specifically you are testing from the patch and whether you need this whole IR output to be checked?

It helps for reviewing and maintenance purposes to minimize the IR patterns as much as possible.

Removed this test.

39 ↗(On Diff #333247)

Removed from all new tests.

clang/test/CodeGenSYCL/address-space-of-returns.cpp
8 ↗(On Diff #333247)

It seems to me that you are just testing address space inference here similar to address-space-deduction.cpp? I don't see anything specific to a return type in either the test or your patch?

Right. Removed.

clang/test/CodeGenSYCL/address-space-parameter-conversions.cpp
80 ↗(On Diff #333247)

You are not testing the mangling though?

Right, what we are checking here is that each instantiation emits a different function in LLVM IR. Exact mangling is not relevant to the check.

clang/test/CodeGenSYCL/convergent.cpp
2 ↗(On Diff #336543)

I see, this looks trivial enough test clean up. You could just commit it without any review perhaps?

Done in https://github.com/llvm/llvm-project/commit/95c614afcd4de71d00a240d6a4a02c036c972ed0

bader edited the summary of this revision. (Show Details)Tue, Apr 13, 9:04 AM
bader edited the summary of this revision. (Show Details)
bader marked 2 inline comments as done.
bader added inline comments.
clang/include/clang/AST/Type.h
488

It seems like they would just be extending the existing functionality and not redesigning what we do in this patch?

If that's the case let's keep it in a separate patch, but feel free to upload it even now.

Added in https://reviews.llvm.org/D100396.

Anastasia added inline comments.Wed, Apr 14, 4:35 AM
clang/lib/AST/ItaniumMangle.cpp
2465

For now I am just trying to understand why you are not adopting similar mangling scheme as for other language address spaces since it gives more stable mangling irrespective from the target compiled for.

If you plan to link libraries from other frontends i.e. OpenCL or CUDA the mangling you use is different from what they produce. Just have a look at the line 2470 that explains OpenCL mangling or line 2494 explaining CUDA mangling. FYI similar scheme applies to other language address spaces, so the AS<num> was only really used for the address spaces that have no source spelling i.e. no language semantics.

clang/lib/Basic/Targets/SPIR.h
131

I mean the FIXME is about the issue with Clang design:
https://reviews.llvm.org/D89909#inline-941733

Basically, just explain why we need to reset the map and that it is only needed for Default address space.

clang/lib/CodeGen/TargetInfo.cpp
10077

This was the original implementation (see https://reviews.llvm.org/D89909?id=299795), but @rjmccall suggested to use this callback instead.

Did you mean to link some particular conversation? Currently, it resets to the top of the review.

Both ways work for me, but the implementation proposed by John is easier to maintain.

I can't see why the same code would be harder to maintain in the caller. If anything it should reduce the maintenance because the same logic won't need to be implemented by every target.

I also added an assertion for SYCL language mode, although I think SPIR doesn't support global variables in address spaces other than global or constant regardless of the language mode, so I think the logic is universal.

Asserts don't guard this logic to be applied universally. And since the IR was generated like this for about 10 years I don't feel comfortable about just changing it silently.

To my memory SPIR spec never put restrictions to the address spaces. It only described the generation for OpenCL C. So if you compile from C you would have everything in the default address space. And even OpenCL rules doesn't seem to be quite accurate in your patch as in OpenCL C globals can be in __global, __constant or __local. However, the SPIR spec was discontinued quite a while ago and the implementation of SPIR has evolved so I am not sure how relevant the spec is now.

Personally, I feel the behavior you are implementing is governed by the language soI think it is more logical to encapsulate it to avoid interfering with other language modes.

clang/test/CodeGenSYCL/address-space-parameter-conversions.cpp
80 ↗(On Diff #333247)

Ok, this is the kind of change that we normally expect tested explicitly. I don't believe you would be ok that we modify that freely as we like after you commit the patch.

bader updated this revision to Diff 337441.Wed, Apr 14, 6:59 AM
bader marked 5 inline comments as done.

Applied more comments.

clang/lib/AST/ItaniumMangle.cpp
2465

For now I am just trying to understand why you are not adopting similar mangling scheme as for other language address spaces since it gives more stable mangling irrespective from the target compiled for.

According to my understanding this code is used for other language spaces. For instance, per comments at lines 2455-2457 it's used for OpenCL and CUDA address spaces.
Do you mean some other mangling scheme?

If you plan to link libraries from other frontends i.e. OpenCL or CUDA the mangling you use is different from what they produce.

SYCL standard doesn't have such functionality. OpenCL C functions are not mangled (only built-ins), so there should be no problem to link with OpenCL C libraries.
I know that mangling difference causes some problems for SYCL built-ins implementations on CUDA, but I don't know all the details. @Naghasan knows about these. @Naghasan, do you have suggestions to fix the problems caused by mangling?

Just have a look at the line 2470 that explains OpenCL mangling or line 2494 explaining CUDA mangling. FYI similar scheme applies to other language address spaces, so the AS<num> was only really used for the address spaces that have no source spelling i.e. no language semantics.

Is this statement correct? https://godbolt.org/z/n7The38dz - AS<num> is used for OpenCL address spaces mangling when it's compiled for spir target.

clang/lib/Basic/Targets/SPIR.h
131

Added FIXME comment.

clang/lib/CodeGen/TargetInfo.cpp
10077

This was the original implementation (see https://reviews.llvm.org/D89909?id=299795), but @rjmccall suggested to use this callback instead.

Did you mean to link some particular conversation? Currently, it resets to the top of the review.

I pointed to the patch version implementing address space deduction in CodeGenModule::GetGlobalVarAddressSpace.
Conversion pointer is RFC in the mailing list: http://clang-developers.42468.n3.nabble.com/RFC-Re-use-OpenCL-address-space-attributes-for-SYCL-tp4068754p4069039.html

Both ways work for me, but the implementation proposed by John is easier to maintain.

I can't see why the same code would be harder to maintain in the caller. If anything it should reduce the maintenance because the same logic won't need to be implemented by every target.

I also added an assertion for SYCL language mode, although I think SPIR doesn't support global variables in address spaces other than global or constant regardless of the language mode, so I think the logic is universal.

Asserts don't guard this logic to be applied universally. And since the IR was generated like this for about 10 years I don't feel comfortable about just changing it silently.

To my memory SPIR spec never put restrictions to the address spaces. It only described the generation for OpenCL C. So if you compile from C you would have everything in the default address space. And even OpenCL rules doesn't seem to be quite accurate in your patch as in OpenCL C globals can be in __global, __constant or __local. However, the SPIR spec was discontinued quite a while ago and the implementation of SPIR has evolved so I am not sure how relevant the spec is now.

Personally, I feel the behavior you are implementing is governed by the language soI think it is more logical to encapsulate it to avoid interfering with other language modes.

Added early exist for non-SYCL modes.

Anastasia added inline comments.Fri, Apr 16, 4:58 AM
clang/lib/AST/ItaniumMangle.cpp
2465

Is this statement correct? https://godbolt.org/z/n7The38dz - AS<num> is used for OpenCL address spaces mangling when it's compiled for spir target.

I am very confident this is correct https://godbolt.org/z/8o6vdYEPc though perhaps comments could be made more clear and feel free to improve them if you find them misleading.

So if you follow the flow here you can see that if the target maps the address space to a distinct non-zero value then it is using the target scheme mangling otherwise it will use the language spelling.

I believe this is done to prevent the name clashing, so if let's say you have overloaded functions:

foo(__global int*){}
foo(__local int*){}

and your target doesn't have the target address spaces (i.e. you would end up with AS0 and AS0 in both) then you would want to mangle the functions differently to avoid generating two definitions with the same name?

I think though this flow still has a flaw for the cases the target would map language address spaces to non-distinct and non-zero values there could still be a clash. Therefore I think using source address spaces would have been more stable but it would reduce the ability to link functions produced from different language modes.

clang/lib/Basic/Targets/SPIR.h
131

Ok, let's be a bit more specific regarding the clang design flaw i.e. I would add something like

Currently, there is no way of representing SYCL's default address space language semantic along with the semantics of embedded C's default address space in the same address space map. Hence the map needs to be reset to allow mapping to the desired value of 'Default' entry for SYCL.

You could also mention that SYCL has the same semantics as CUDA which would hint that this logic should be generalised to multiple languages not just SYCL.

140

This needs a language guard too?

clang/lib/CodeGen/TargetInfo.cpp
10077

I pointed to the patch version implementing address space deduction in CodeGenModule::GetGlobalVarAddressSpace.
Conversion pointer is RFC in the mailing list: http://clang-developers.42468.n3.nabble.com/RFC-Re-use-OpenCL-address-space-attributes-for-SYCL-tp4068754p4069039.html

This looks actually very neat and I can't see that anyone had any concerns about this.

I think John's comment on RFC is to point out that there are also Target hooks available should you need to override the language semantics but there is no statement that you should prefer it instead of implementing the language rules. I think the language semantics should take precedence.

Added early exist for non-SYCL modes.

To improve the understanding I would prefer if you guard the logic with if statement and return the original address space as default right at the end:

if (CGM.getLangOpts().SYCLIsDevice) {
// do what you need for SYCL
}
// default case - just return original address space
return AddrSpace;
10081

Can you add an error message string to the assert?

Also should you just assert that that address space is 0 i.e. not specified yet? Otherwise I don't think you should override it. Although you actually handle this case below as a valid input? Do you actually need this assert then?

10086

Do you need to query the target from CGM? It returns TargetInfo which member you are overriding here.

https://clang.llvm.org/doxygen/classclang_1_1CodeGen_1_1CodeGenModule.html#a09a815215ff67d2e52b7605af35c40cb

However, I would just change this code to avoid generalizing since the generalization is SYCL specific anyway. The SPIR target does have a constant address space segment, so I would find it strange that it maps to global as a general rule. Why not to make it simple and just return global address space for SYCL in all cases?