[PATCH] D89909: [SYCL] Implement SYCL address space attributes handling

Anastasia Stulova via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Fri Dec 11 05:06:37 PST 2020


Anastasia added a comment.

In D89909#2442573 <https://reviews.llvm.org/D89909#2442573>, @bader wrote:

> In D89909#2439837 <https://reviews.llvm.org/D89909#2439837>, @Anastasia wrote:
>
>> In D89909#2423750 <https://reviews.llvm.org/D89909#2423750>, @bader wrote:
>>
>>>> It was mentioned that the changes in the type system with address spaces is undesirable for SYCL because you indend to parse existing C++ code as-is. This contradicts the intended semantic of address spaces where the whole point of it is to modify the standard types and therefore a compilation of C++ with the standard semantic is only guaranteed when the attribute is not used at all.
>>>
>>> Right, but I don't think it's related to the address space attributes. It was mentioned in the context of re-using OpenCL *mode* for SYCL device compilation, which modifies types which does use address space attribute explicitly. "Existing C++ code" doesn't use address space attributes and our solution does differentiate explicitly annotated type. The difference with OpenCL mode is that SYCL doesn't change types by modifying "default" address space attribute and allows conversion from/to "default" address space. As I mentioned in RFC, according to my understanding this patch doesn't contradict Embedded C requirements regarding address space attribute usage. I think the spec allows an implementation to define conversion rules between address spaces and doesn't imply type change based on the declaration scope - it's OpenCL specific behavior.
>>
>> Ok, if you plan to follow the Embedded C semantic (which your current patch confirms) then you should just reuse the existing target address spaces and extend the implementation with the ability to specify the relation among address spaces. The patch that you have mentioned earlier https://reviews.llvm.org/D62574 is providing this logic already and it looks good aside from testing. I would suggest to discuss with @ebevhan the timeline for committing it as the testing could be done using SYCL implementation. Alternatively, you could consider reusing the relevant parts of the patch if @ebevhan has no objections to that.
>
> Let me check that I understand your proposal correctly.
> Do suggest that we use `__attribute__((address_space(N)))` attribute and define the relation between the set of these? Unfortunately I won't work because SYCL supports multiple targets and we need to use address space maps to correctly map attributes in LLVM IR. Targets might use different different llvm address spaces numbers for memory accessible within a work-item. If I understand it correctly, `__attribute__((address_space(N)))` will be mapped to `addrspace(N)` in LLVM and this mapping can't be customized for different targets. Right?

Yes, this is true. However your libraries can have vendor dependent definitions of the target address spaces i.e. the exact number can be set per vendor. Then you would still need to specify the relationship of address spaces for each vendor in clang. I don't know whether this matches well though. It feels more to me that your concept is closer to address spaces defined by language dialects like OpenCL. If you prefer to continue this route then you should just document your dialect sematic somewhere publicly accessible and avoid aliasing to OpenCL, or target address spaces. So it would become a proper language dialect implementation.

> I actually proposed a bit different approach in RFC. https://reviews.llvm.org/D62574 "moves QualType/Qualifiers accessors which deal with qualifier relations (such as compatiblyIncludes, etc.) to ASTContext, as Qualifiers cannot be made aware of the relations between address spaces on their own". It allows customization based on the language mode as well, so my suggestion was to set "default" address space as a superset for opencl_global, opencl_local and opencl_private in SYCL mode. Using this approach we re-use existing attributes and don't impact other language modes. Does it make sense to you?

The default address spaces in clang is used for flat addressing of C and C++.  Although in early OpenCL versions we have used it for `opencl_private`, that mainly aligned with the concept of Embedded C until in OpenCL 2.0 the semantic of address spaces has been modified significantly. So currently I would say none of the OpenCL address space semantic matches the default address space of clang.

Btw in the original RFC of  https://reviews.llvm.org/D62574, we have also discussed to extend the concept of target address spaces on language dialects that would follow embedded C semantic but the address spaces would work on a variety of targets. We were even thinking of defining a language syntax that would allow expressing the relationship between the address spaces in the source code parsed by clang (for example it could be defined in the header that is included implicitly). Then the dialects could define address space semantic completely outside of the clang code base. It sounds like something that could be useful for SYCL. If you are interested we can elaborate further on this idea, but it would require creating some fare bit of common infrastructure before it could be used for any concrete dialect.



================
Comment at: clang/lib/Basic/Targets.cpp:577
-  case llvm::Triple::spir: {
-    if (Triple.getOS() != llvm::Triple::UnknownOS ||
-        Triple.getEnvironment() != llvm::Triple::UnknownEnvironment)
----------------
bader wrote:
> Anastasia wrote:
> > We should not allow any arbitrary OS/Environment for SPIR.
> @Anastasia, could you elaborate on that, please? Why can't we allow arbitrary values for these triple components?
> 
> The only reason I'm aware of is that LLVM-SPIRV-Translator was checking OS triple component and rejected LLVM module if value was not "unknow". Today it checks only "architecture' component and accepts only "spir" architecture. I personally think that even this restriction is artificial and LLVM module shouldn't be rejected based on the triple values, but it's a separate discussion.
> 
> I use additional environment component to choose between address space maps. We could update default address space in SPIRAddrSpaceMap directly, but IIRC, it caused some issues for OpenCL C++ mode, which uses Default address space instead of `opencl_private` in some cases.
> 
> Do you know if there is a better way to communicate the difference in Default address space to SPIRTargetInfo object?
SPIR is not a real target but a virtual one and therefore it doesn't have different flavour for vendors or  and not run with any concrete OS. What good would it make to allow specifying `spir-apple-darwin10` or `spir-pc-win32`? What difference do we expect?

> Do you know if there is a better way to communicate the difference in Default address space to SPIRTargetInfo object?

If you only add the environment component to set the address spaces map then it doesn't feel like the right flow. In fact, your comments indicate that you aim to support the other targets too - how would this work for other targets that are not SPIR? And why are you specializing the map for SPIR but not other targets? 


================
Comment at: clang/lib/Basic/Targets/SPIR.h:71
     LongWidth = LongAlign = 64;
-    AddrSpaceMap = &SPIRAddrSpaceMap;
+    if (Triple.getEnvironment() == llvm::Triple::SYCLDevice) {
+      AddrSpaceMap = &SYCLAddrSpaceMap;
----------------
bader wrote:
> Anastasia wrote:
> > This deserves clarification - address space map reflect the physical address space of the target - how will upstream users get to physical address spaces of targets for SYCL?
> The same way. The only difference for SYCL is that "Default" language address space is mapped to "4" LLVM AS, which is equivalent to "opencl_generic" language address space.
If you are aiming at following the Embedded C semantic then this change is not legitimate. In Embedded C generic address space corresponds to no address space:

> When not specified otherwise, objects are allocated by default in a generic address space, which corresponds to the single address space of ISO/IEC 9899:1999

I.e. in LLVM project default address space is a flat address space of C/C++. You should either inherit it as is or use a different entry in the map... As a matter of fact, OpenCL generic address space is not the same as the generic address space of C and C++. It is only used for the pointers.


================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:10035
+  if (CGM.isTypeConstant(D->getType(), false)) {
+    if (auto ConstAS = CGM.getTarget().getConstantAddressSpace())
+      return ConstAS.getValue();
----------------
bader wrote:
> Anastasia wrote:
> > You should not have constant address space in SYCL!
> According to my understanding it's not about "constant" address space like in OpenCL. This method returns LLVM address space for constants (like literals: `56`, "hello").
> 
> I think it might be better to split this patch into two and move enabling regular C++ for SPIR to a separate patch. I'll try to make this change if there are objections.
Ok, I see, makes sense. I don't mind either way.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89909/new/

https://reviews.llvm.org/D89909



More information about the cfe-commits mailing list