[SYCL] Implement SYCL address space attributes handling

[bader]

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.

[Anastasia]

clang/lib/AST/ItaniumMangle.cpp

2379

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.

127

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

9985

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

9

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.

40

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

12 ↗

(On Diff #336543)

Should you check that the function name has expected mangling?

clang/test/CodeGenSYCL/address-space-of-returns.cpp

9

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]

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

81

You are not testing the mangling though?

125

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

2

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

26

Sorry I mean from Default to named AS.

[bader]

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]

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

127

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]

Applied more code review suggestions.

Rebased on ToT.

[bader]

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

2379

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

127

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

9985

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

9

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.

40

Removed from all new tests.

clang/test/CodeGenSYCL/address-space-of-returns.cpp

9

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

81

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]

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]

clang/lib/AST/ItaniumMangle.cpp

2379

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

127

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

9985

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

81

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]

Applied more comments.

clang/lib/AST/ItaniumMangle.cpp

2379

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

127

Added FIXME comment.

clang/lib/CodeGen/TargetInfo.cpp

9985

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]

clang/lib/AST/ItaniumMangle.cpp

2379

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

127

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.

136

This needs a language guard too?

clang/lib/CodeGen/TargetInfo.cpp

9985

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;

9989

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?

9994

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?

[bader]

clang/lib/CodeGen/TargetInfo.cpp

9985

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.

Do I understand it correctly that you suggest replacing Target hooks with the CodeGen library changes from the first version of the patch?
@rjmccall, are you okay with that?

[Anastasia]

clang/lib/CodeGen/TargetInfo.cpp

9985

Yes, that's right. I suggest lifting this logic into CodeGenModule::GetGlobalVarAddressSpace, so something like

diff --git a/clang/lib/CodeGen/CodeGenModule.cpp b/clang/lib/CodeGen/CodeGenModule.cpp
--- a/clang/lib/CodeGen/CodeGenModule.cpp
+++ b/clang/lib/CodeGen/CodeGenModule.cpp
@@ -3849,6 +3849,12 @@
     return AddrSpace;
   }
 
+  if (LangOpts.SYCLIsDevice) {
+    if (!D || D->getType().getAddressSpace() == LangAS::Default) {
+      return LangAS::opencl_global;
+    }
+  }
+
   if (LangOpts.CUDA && LangOpts.CUDAIsDevice) {
     if (D && D->hasAttr<CUDAConstantAttr>())
       return LangAS::cuda_constant;
@@ -3874,8 +3880,19 @@
   // OpenCL v1.2 s6.5.3: a string literal is in the constant address space.
   if (LangOpts.OpenCL)
     return LangAS::opencl_constant;
+
+  // If we keep a literal string in constant address space, the following code
+  // becomes illegal:
+  //
+  //   const char *getLiteral() n{
+  //     return "AB";
+  //   }
+  if (LangOpts.SYCLIsDevice)
+    return LangAS::opencl_private;
+
   if (auto AS = getTarget().getConstantAddressSpace())
     return AS.getValue();
+
   return LangAS::Default;
 }

from your original patchset.

[bader]

clang/lib/CodeGen/TargetInfo.cpp

9985

Okay. Just to make sure that all details are clear:

• We drop all SPIRTargetCodeGenInfo changes and getConstantAddressSpace for SPIR target.
• SYCL language specific changes in CodeGen are not limited to CodeGenModule. As you might notice CodeGenModule modifications lead to address space changes, which are not expected by other part of the library. We need update bitcast with addrspacecast instruction in a few places to. Basically it means "take CodeGen library changes from the first patch".
• Most of the test cases removed during code review cover the cases mentioned in the previous item and should be added back.

Let me know if there any concerns with any of these items.

[Anastasia]

clang/lib/CodeGen/TargetInfo.cpp

9985

SYCL language specific changes in CodeGen are not limited to CodeGenModule. As you might notice CodeGenModule modifications lead to address space changes, which are not expected by other part of the library. We need update bitcast with addrspacecast instruction in a few places to. Basically it means "take CodeGen library changes from the first patch".

I don't see how moving the same logic elsewhere would need extra address space casts? Your SPIR target changes have identical semantics to the diff I pasted above.

[bader]

clang/lib/CodeGen/TargetInfo.cpp

9985

Probably I misunderstand your proposal. Do you suggest moving SPIRTargetCodeGenInfo::getGlobalVarAddressSpace to CodeGenModule::GetGlobalVarAddressSpace, but keep the rest of SPIRTargetCodeGenInfo and SPIRTargetInfo changes?

[Anastasia]

clang/lib/CodeGen/TargetInfo.cpp

9985

Yes, indeed.

[bader]

Applied more review comments.

[bader]

Added SYCL address spaces mangling for targets without address space map

[bader]

clang/lib/Basic/Targets/SPIR.h

136

I can re-write this method to avoid using language address space:

llvm::Optional<LangAS> getConstantAddressSpace() const override {
  return getLangASFromTargetAS(1);
}

Does it look okay to you?

I don't think we need a language guard here as this hook is already guarded by users. E.g. https://github.com/llvm/llvm-project/blob/main/clang/lib/CodeGen/CodeGenModule.cpp#L4137-L4159.
Adding language guards for TargetInfo::getConstantAddressSpace method will require API change similar to adjust method i.e. explicit LangOptions type parameter.

[Anastasia]

clang/include/clang/AST/Type.h

493

Do you still plan to wait for extra input or otherwise we could just update the documentation for now?

If you discover that you need to allow the reverse conversions later it should not be problematic to add since it won't break anyone's code. It will only allow more code to compile!

clang/lib/Basic/Targets/SPIR.h

136

Well, OpenCL is not the only language mode though so it generally is an ABI change. But also there are seem to be other uses of this function, for example in CGExpr.cpp that are not guarded by the language mode and there it seems like constant address space might make more sense for OpenCL.

I think the situation is similar here as for global variables - we might need a constant alternative to CodeGenModule::GetGlobalVarAddressSpace. But for the time being if you only need this logic for the purposes of castStringLiteralToDefaultAddressSpace why not to just add a SYCL special case straight there? I don't think your current implementation would allow using any other address space for this anyway?

[bader]

clang/include/clang/AST/Type.h

493

Okay. I'll update the document right away.

clang/lib/Basic/Targets/SPIR.h

136

This logic should be applied to all types of literals, so changing only castStringLiteralToDefaultAddressSpace is not enough. I think this is where the most of the CodeGen changes from the first version of the patch are coming from (if not all of them). I'll check if it can be refactored to something like CodeGenModule::GetGlobalVarAddressSpace.

[bader]

Generalize getStringLiteralAddressSpace to GetGlobalConstantAddressSpace

Rebased on ToT.

[bader]

@Anastasia, I've updated https://reviews.llvm.org/D99488 and refactored getStringLiteralAddressSpace to handle non-string constants as well. Please, take a look.

[Anastasia]

LGTM! Thanks!

[aaron.ballman]

Just one drive-by nit from me.

clang/lib/CodeGen/CodeGenModule.cpp

4119–4123 ↗

(On Diff #339973)

Just simplifying the logic a bit, likely needs to be reformatted though.