Differential D89909
[SYCL] Implement SYCL address space attributes handling Authored by bader on Oct 21 2020, 1:34 PM.
Details Default address space (applies when no explicit address space was Added SYCL named address spaces sycl_global, sycl_local and Static variables without address space now reside in global address
Diff Detail
Unit Tests
Event TimelineThere are a very large number of changes, so older changes are hidden. Show Older Changes
Comment Actions The documentation looks like a good starting point but it would be better to add it to clang docs, then we can also refine the details during the review. For now I can tell that the following points are too high-level and unclear:
You should provide more details on the difference to OpenCL address spaces - in particular looking at your patch I don't think this statement is correct "unmodified <...> IR generation components."
I don't understand what integration you mean. Also C++ has many different kinds of conversions, which conversions do you mean exactly? Otherwise, the overall design of address spaces seems a lot more clear now. Regarding SYCLDevice and SYCLAddrSpaceMap I am still not very convinced about the flow. Have you had any design discussion regarding this already that you could point to? My main concern is that I am not sure it fits with the main design of Clang - producing IR that is to be consumed by multiple targets rather than only one specific target or even an external tool. Why do you add a triple component that is used only for one target SPIR:
I also think it would be good if @rjmccall could look at the Codegen changes.
Comment Actions
We discussed this with you in https://github.com/intel/llvm/pull/1039/.
High level languages differences doesn't matter for toolchains consuming LLVM IR - it can python, D or SYCL. Toolchains are assuming that LLVM IR follows the sematic defined in SPIR-1.2 document - https://www.khronos.org/registry/SPIR/specs/spir_spec-1.2.pdf.
The main reason is the difference in handling Default address space in C++ for OpenCL mode and SYCL. C++ for OpenCL doesn't always deduce address space and there are cases when Default is used and SPIR target maps it AS 0 (equivalent of SPIR-V Function storage class). This lead to inconsistencies like reported here: https://bugs.llvm.org/show_bug.cgi?id=45472. SYCL doesn't deduce language address space at all relies on mapping Default in LLVM AS equivalent of SPIR-V generic. NOTE: other GPU targets like AMDGPU and NVPTX do the same i.e. maps Default language AS to LLVM IR AS equivalent of SPIR-V generic, so there is no need to update. I believe SPIR target must apply the same mapping.
We use alternative map to not impact OpenCL language modes. I provided clang lit tests results for changing the default map below. Most of the failures are caused by valid changes in LLVM IR and can be fixed by updating FileCheck comments, but there are a few crashes in CodeGen for "C++ for OpenCL" mode (with the root cause similar t o https://bugs.llvm.org/show_bug.cgi?id=45472). AFAIK, it's the only blocker for using unified map for any language mode. @svenvh, do you think we can address https://bugs.llvm.org/show_bug.cgi?id=45472 and update SPIRAddrSpaceMap (i.e. SPIRAddrSpaceMap[0] <- 4)? Failed Tests (22): Clang :: CodeGen/ext-int-cc.c Clang :: CodeGen/spir-half-type.cpp Clang :: CodeGenCXX/builtin-calling-conv.cpp Clang :: CodeGenOpenCL/addr-space-struct-arg.cl Clang :: CodeGenOpenCL/atomic-ops-libcall.cl Clang :: CodeGenOpenCL/blocks.cl Clang :: CodeGenOpenCL/intel-subgroups-avc-ext-types.cl Clang :: CodeGenOpenCL/opencl_types.cl Clang :: CodeGenOpenCL/partial_initializer.cl Clang :: CodeGenOpenCL/private-array-initialization.cl Clang :: CodeGenOpenCL/sampler.cl Clang :: CodeGenOpenCL/vectorLoadStore.cl Clang :: CodeGenOpenCLCXX/address-space-castoperators.cpp Clang :: CodeGenOpenCLCXX/address-space-deduction.cl Clang :: CodeGenOpenCLCXX/addrspace-derived-base.cl Clang :: CodeGenOpenCLCXX/addrspace-of-this.cl Clang :: CodeGenOpenCLCXX/addrspace-operators.cl Clang :: CodeGenOpenCLCXX/addrspace-references.cl Clang :: CodeGenOpenCLCXX/addrspace-with-class.cl Clang :: CodeGenOpenCLCXX/atexit.cl Clang :: CodeGenOpenCLCXX/template-address-spaces.cl Clang :: Index/pipe-size.cl
Comment Actions I can't see.
I can't understand what you mean by "doesn't matter"? If you compiler from C++ you get one address space as Default and if you compile from SYCL you get a different one for the same target - how do you expect the toolchain consuming the IR to handle that? Certainly this is not how LLVM was designed. LLVM expects the same address spaces to be used with one target for all languages. FYI, the document is no longer describing the current SPIR target adequately as implementation deviated quite a bit from the original documentation.
Generic address space in Clang is a logical concept added for OpenCL to implement the following logical concept . Targets are allowed to map it to any physical address space (it is very reasonable to map it to Default) but it is only used for restricted use cases i.e. pointers or references. Default address space is where objects are put by default - for example in C++ everything is mapped to this address space. I don't see why you just replace Default by OpenCL generic. It has been added for a very different purpose. Comment Actions The mapping has been discussed in this comment: https://github.com/intel/llvm/pull/1039/#discussion_r369667791.
LLVM has it's own sematic which toolchains rely on. Toolchains consuming LLVM IR doesn't know if this LLVM IR was emitted by C++ compiler or written manually - they can rely only on LLVM IR semantic defined by https://llvm.org/docs/LangRef.html.
Technically clang changes SPIR target mapping for Default address space only for sycldevice environment component of the target triple, so it doesn't depend on the input language i.e. Default is mapped to the same LLVM address space for C++ and SYCL.
This is unfortunate. Do you know if there are any plans to provide up-to-date documentation? It's critical for non-clang front-ends targeting SPIR-V format.
I agree with that, but I don't get how it's related to the mapping language AS to LLVM AS.
SPIR-V Generic is the only address space allowing us to preserve correct semantic of the program (possibly global can be used, but we didn't investigate this option due to obvious performance implications). The objects from Default AS might be allocated in any physical AS, so LLVM IR AS 4 allows LLVM compiler to infer it using InferAddressSpace pass. Comment Actions The discussion you cite is in the separate Github project, but you should have a discussion using LLVM channels about the design you propose for llvm-project repo. Also I don't think citing resources with many comments and no conclusions is making the review process very productive.
If you are suggesting to add SYCL as a component for other languages supported by clang this deserves a separate review probably with different reviewers and a wider discussion with the community.
I am not aware of any plan. FYI SPIR has not been added or designed for SPIR-V. It is only used for SPIR-V as a workaround by some external projects but it is not the only use case even if perhaps an important one for the community.
Every language address space has its own semantic that should be mapped appropriately to LLVM IR to make sure the semantic is preserved. OpenCL generic is a concept governed by OpenCL C spec cited above It can be used with pointer types and it represents a placeholder for any of the named address spaces - global, local or private. It signals that a pointer points to an object in one of these concrete named address spaces. The exact address space resolution can occur dynamically during the kernel execution. and it is completely different to the concept of Default that is a flat address space of C and C++ as per Embedded C s5.1.1 For the purpose of this Technical Report, the C language is extended to support additional address spaces. 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. In addition to the generic address space, an implementation may support other, named address spaces. I hope you see the difference between the two. As SPIR is a virtual target, Default and OpenCL generic have distinct values because you need to preserve their semantic until the mapping to the physical device. I don't see why these two concepts should have the same logical mappping? For the generality tools consuming SPIR LLVM IR should have information for both address spaces and then decide how to represent or map those to a physical device. Then on a physical device target it makes sense that some or even all address spaces are mapped to the same value representing physical segments.
FYI LLVM doesn't support SPIR-V yet, but nevertheless SPIR-V generic has the same semantic as OpenCL generic but it is not the same as Default in clang.
As far as I am aware you can also use any value with InferAddressSpace pass not only 4. Comment Actions
Sorry, I forgot to mentioned that this change was also discussed in the mailing list: http://clang-developers.42468.n3.nabble.com/RFC-Re-use-OpenCL-address-space-attributes-for-SYCL-td4068754.html. What is not covered by this thread and probably worth to discuss separately is changing OpenCL private mapping to non-zero LLVM AS to avoid the mangler component change. I expect significant impact on SPIR LLVM IR consumers and long transition period if the change is accepted.
No, but clang doesn't couple input language with environment component of the target triple. As I mentioned above, community doesn't have any objections to this approach.
I'm aware of SPIR target history and design - I was on the team contributed SPIR-1.2 support to clang, although I was involved into compiler back-end development.
I puzzled by existing mapping: Aren't Default and SPIR-V Function (or OpenCL private) semantically different address spaces? FWIW, real GPU targets map Default to Generic and as far as I concerned it doesn't cause problems.
Right, but we use SPIR target and it uses 4. Comment Actions
I remember asking similar questions regarding the SYCL address space map on that thread, and I don't think I could get more than what we are discussing here. I generally don't think that RFC ended with any conclusions as a result this patch contained different approach to what I have suggested at the end.
I would like to highlight that SPIR was completely replaced by SPIR-V. I don't really see it as evolution. There were several attempts to add SPIR-V into LLVM and Clang - both of us were involved in some of those. As of now the comments in the source code still says: llvm/include/llvm/ADT/Triple.h: spir, // SPIR: standard portable IR for OpenCL 32-bit version llvm/include/llvm/ADT/Triple.h: spir64, // SPIR: standard portable IR for OpenCL 64-bit version
I presume it is used for what SPIR was originally intended - a de-facto portable format? As a matter of fact not all OpenCL vendors support SPIR-V and we as a community won't and can't force this. If you want to learn more about use cases I would ask the community, cfe-dev could be a good start.
Yes they are. OpenCL private is just the most common case this is why mapping them to the same value often works.
Default can be used to allocate objects even if address spaces for alloca can be customized. There are other objects that are used with Default - arguments or return types. It is just so inherent in the parser. Most of objects use Default at the AST level even if not all of this ends up in the IR.
Sorry I don't think I have enough time. But in general I think you should not base the design decision solely on the examples as both C and C++ are complex enough to overlook various corner cases. Your design should make sense conceptually and then the tooling will be correct by construction and easy to reason about.
How do you prove this though?
I guess I still don't get your architecture, but you could also look at adding a pass that remaps the address spaces or extend the pass to work with different default address spaces? Comment Actions Just a few minor nits from me, but I'm mostly wondering: where are we at with this and are there still substantive changes required? (I looked through the comments, but there's a lot of back-and-forth since Oct and I'm not certain what's holding the patch back currently.)
Comment Actions To make it short, from my side I am not very clear about the overall design. From the SYCL spec side, there is no indication of what compiler extensions are needed and if at all. As a result, some of the design choices are unclear to me - in particular why SPIR target would need a separate address space map for SYCL. This is not how it was intended originally and I am worried that this will create issues for the consumers of IR to handle two different formats. But in general, if the community is now to maintain this code we should at least have some deeper understanding of it. I would suggest starting from some high-level documentation that provides the details of the compiler extension being implemented. Perhaps the documentation that @bader has linked earlier could be used as a starting point with some more details that would allow assessing and reviewing the changes. Comment Actions @Anastasia, do you suggest we copy https://github.com/intel/llvm/blob/sycl/sycl/doc/CompilerAndRuntimeDesign.md document to clang/docs within this patch? Comment Actions For the purpose of this specific topic, you could indeed move "Address spaces handling" section into the clang documentation. I would suggest creating a dedicated page where you could gather SYCL specific internals for the clang community. You could also link the github page to it for more comprehensive details. I would recommend extending the documentation slightly to focus on what behavior is expected to be implemented rather than how you propose to implement it in clang too. This is a general guideline for the clang contributions that suggest that the documentation should be detailed such that it would be possible to implement it in other frontend/compiler. You could for example include some more information on expected language semantic i.e. what you inherit from embedded C and what you inherent from OpenCL or any other available language features with relevant spec/doc references including SYCL spec. This should facilitate anyone who needs to understand the implementation to find further details. It would probably make sense to create a separate review to avoid adding too much noise here. Once you create a review we can link it here and also refine any necessary details as we go along. Comment Actions These all seem like good suggestions, from my limited perspective, so thank you for them! I'm not opposed to the documentation requests, however, I think that is work that is separable from the proposed changes in this review and could be done with post-commit follow-ups, unless you think the current documentation is wrong as it relates to this patch. I worry that back-and-forth on documentation clarity (while certainly important) is going to hold this patch up for several more months, which is a burden. Comment Actions Just to be clear I am not suggesting a nice-to-have documentation. I would like to see the documentation that explains what behavior is expected from the compiler. I think this is normally a good starting point and it has been a common practice for many contributions I believe too. I guess we could first commit the implementation and then work out what we want it to be doing but that implies a bigger risks to the community. Also I am also not entirely happy with some directions taken deviating the original design in this patch. But it is hard to assess whether there are better practical alternatives and suggest anything without understanding what is being implemented. Address spaces are used by many stakeholders in clang so having a clear understanding of various use cases publicly accessible would benefit the community and reduce the risks of disruptions and negative impact of each other's work. Regarding the timing I believe it could take less than several months considering that there are some parts already available but it depends on various factors including everyone's availability. However, the fact that some areas would require collaborative exploration of design alternatives from stakeholders was to my memory highlighted on the original proposal to contribute SYCL support to clang quite a while ago. So the requirement here should not appear as unexpected. This has been my suggestion to address the ping-ponging discussion issues and latencies/time incurred by them as I think it would be good for us to get the terminology sorted. But I am happy if there are other suggestions addressing the concerns. Comment Actions If what you're suggesting is that Clang have a SYCL-specific page that serves a similar purpose to https://clang.llvm.org/docs/OpenCLSupport.html, then I agree we should have that (and it should be linked to from https://clang.llvm.org/docs/index.html the same as OpenCL, OpenMP, etc). However, I think that's orthogonal to this patch and should be done separately. (If the documentation already existed, then I'd request that this patch update it, but because those docs don't exist yet, I think it's unrelated enough to warrant being done separately.) If you're requesting something different, I'd appreciate a bit more specific details.
Given that we lack the dedicated documentation page for SYCL within Clang, I guess I view that as the status quo (we've already started some of the upstreaming work and none of it is documented in a convenient place for Clang users). Based on that, as someone who doesn't really do much with SYCL to date, I think it's reasonable to accept this patch because it extends existing functionality in a straight-forward manner with a reasonable explanation as to why.
If there are technical concerns to be addressed, I'm not certain it's clear (to me, at least) what they are.
That's reasonable, but I think we also have to trust the domain experts who are submitting patches to have done their design homework and to be willing to address concerns as they come up when there's concretely a problem. This is not an extremely experimental invention from an unknown lone contributor; this is a long-time maintainer upstreaming work from downstream repo with users who use this functionality.
Strongly agreed! But my point is: it seems like this is (at least to some extent) already publicly accessible information and I don't think we've identified any disruptions or negative impacts (yet).
I read this as "Maybe it won't take several more months depending on factors outside of the patch author's control" which doesn't really address my concerns that there's not a clear direction on how to unblock this patch in a timely fashion. That's not to imply that I think your concerns are invalid or should be hand-waved away! I agree that we should make sure there's comfort with the design and try to reduce risk as much as we can. It's more that I think I'm recognizing that trying to gain this clarity over Phabricator with long latency on the discussion is somewhat ineffective. Based on the discussion so far, would these be acceptable steps to take? 0) Complete review on this patch for any technical concerns related to it and commit when it's ready (this unblocks some downstream needs quickly, hopefully).
Some of these steps can be done in parallel, of course. I recognize we could have the order be #1 -> #3 -> #0 (so the docs for this change are written as part of this patch), but my concern with that approach is that this patch is blocking other SYCL efforts (@bader can correct me if I'm wrong about this) and no one has identified a definite issue with it yet beyond the lack of Clang documentation. If those steps seem unreasonable or like I've totally missed the point on something, perhaps we could reach an understanding more quickly via a meeting (we could summarize the decisions from the meeting here so the community is aware of the end results)? Comment Actions
I don't really mind the format. It doesn't have to be in Clang documentation although perhaps it makes the most sense logically. What I would like to see is a transparancy within the commnity that is I believe a common practice as per clang contribution policy: A specification: The specification must be sufficient to understand the design of the feature as well as interpret the meaning of specific examples. The specification should be detailed enough that another compiler vendor could implement the feature. I don't find it great to continue reviewing the work where I don't have a clear picture. I also would like to avoid the situation the community has to work with code that we don't understand. We should not be in need to reverse engineer what is being implemented. It is not a comfortable position to be nor it is productive. Once we know the high-level language semantic it facilitates us to reason about implementation should we need to redesign anything or fix a bug. If we only have fragements of code it doesn't tell us a lot and there is very little we can do with it. Perhaps this is why good practices have been added by the community? There are also other areas whether it demonstrated to work really well - for very good reasons we no longer add undocumented attributes to clang.
I am not aware of all work on SYCL, but I believe some documentation was provided, for example, for the new kernel attribute? Personally I don't find the patch straight-forward mainly because it deviates from the original design wrt target address space map. But there are other concerns - for example the need for InferAddressSpace that is not required for embedded C and C++ functionality that to my understanding SYCL aims to implement. However, I don't know whether my understanding of the aim is correct as I don't have any reference to verify my understanding.
Well the issue I got stuck at was highlighted earlier - the need to add new address spaces map for SYCL. So far the design we were striking at is that every language would have a new entry in the map but adding multiple maps per language seems to defeat the purpose. Considering that OpenCL and SYCL would likely be supported by similar targets we should try to avoid this or at least try to understand why we ended up with such situation and what we might be able to do to improve. I would accept if we can't do anything with this immediately in case it requires big restructuring but perhaps at least this deserves a wider community communication.
I don't think this can be called a trust issue. For me personally, it is about making sure to align with the requirements from various domains that none of us has complete knowledge of. There is certainly no invention but however a case of substantially new functionality that is using the feature scattered all over the frontend source code. I am sorry that I just don't find it trivial. While I have worked on this topic for many years I wouldn't trust myself to make a new design decision alone because it is important to get things right in general not just for my product. As a matter of fact me and some other developers from this area were working quite a lot in the last years on the unification of the functionality for different dialects which results in better understanding and less maintenance.
This is somewhat unfortunate because the fact that collaboration would be needed in this area has been highlighted straight from the beginning. Technically we would only need #3 to unblock #0 as creating a new page is very trivial and it could be done as a part of documenting the SYCL address space language sematic. I believe that with the use of references to existing documentations - SYCL, OpenCL, embedded C specifications, etc, such a goal can be accomplished without significant time investment.
I am not opposed to this, but it would be good to agree on the exact agenda before to ensure we can make some progress. Comment Actions [NFC] Align address space map names with AMDGPU target. Rename SPIRAddrSpaceMap and SYCLAddrSpaceMap to SPIRDefIsPrivMap and SPIRDefIsGenMap. Comment Actions Let's take a look at compilation output for the following program: c++
void foo(int *p) {}OpenCL compiler produces following LLVM IR for SPIR target: LLVM
define dso_local spir_func void @_Z3fooPU3AS4i(i32 addrspace(4)* nocapture %0) local_unnamed_addr #0 {
ret void
}Here is what SYCL device compiler produces: LLVM
define dso_local spir_func void @_Z3fooPi(i32* nocapture %0) local_unnamed_addr #0 {
ret void
}We would like to get equivalent code produced by both compiler for SPIR target. I updated address space map names to avoid confusion that mapping change is specific to the language mode. Now they use the same naming scheme as AMDGPU maps. Using InferAddressSpace pass is not required for the targets supporting SPIR-V with pointers to generic address space, but it can be beneficial to produce more efficient native code. I also created a review request with SYCL design documentation - https://reviews.llvm.org/D99190 Comment Actions
Well AMDGPU is a specific target in contrast to SPIR. But the question is whether the map that has been renamed is only intended for SYCL or can it be used for OpenCL or other languages too? My worry is that the original design was intended to separate language and target information but somehow it is now driven towards tangling them together more and more.
Ok, that's good to know that it is only intended for optimisations!
Thanks. I will take a look at the address space part. My first impression is that it would be good to abstract away from the implementation in clang and start from the language semantic that you are trying to get. Perhaps it will be easier if I provide concrete questions on the review. Comment Actions @aaron.ballman, I've uploaded SYCL architecture design document with address space section (https://reviews.llvm.org/D99190). Yesterday, we chatted offline and agreed that the main issue is missing documentation for Clang extensions being added for SYCL. To address this issue we are adding SYCL architecture design document, which we are going to update along with adding new features. @Anastasia, @aaron.ballman, is there anything I can do for this patch to be accepted? Comment Actions I have provided my feedback regarding the address space section on the documentation review. I don't think I will be able to look at the other parts of documentation though. So if you would like to finalize the address spaces faster it might make sense to split it up into a separate review. This might also help to reduce the noise. Comment Actions
I'd like to provide a bit more detailed summary. I think meeting was very useful with the following basic outcome:
Do you agree on the list above or I missed anything? This is what has been done already after the meeting based on the comments made:
I believe that currently this PR is in much better shape than it was and I think I addressed all critical missing items that have been mentioned.
Comment Actions Applied code review suggestions. Rebased on ToT and updated commit message.
Comment Actions Applied more comments.
Comment Actions Generalize getStringLiteralAddressSpace to GetGlobalConstantAddressSpace Rebased on ToT. Comment Actions @Anastasia, I've updated https://reviews.llvm.org/D99488 and refactored getStringLiteralAddressSpace to handle non-string constants as well. Please, take a look. Comment Actions Just one drive-by nit from me.
| |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
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?