[SYCL][Doc] Design doc for compile-time properties #4937
Conversation
Add a design document for the DPC++ extension API `SYCL_EXT_ONEAPI_PROPERTY_LIST`, which describes how compile-time properties are recognized by the front-end, how they are represented in LLVM IR, and how they are translated into SPIR-V.
AlexeySachkov
requested review from
romanovvlad,
premanandrao,
elizabethandrews and
vladimirlaz
|
@AlexeySotkin could you review this from the SPIR-V perspective? I think the section "Representing properties in SPIR-V" is most relevant to you. |
Do I get it right that this design aims to avoid any changes in the compiler FE, while on SPIR-V level we still need to create a SPIR-V extension every time the desired property can't be expressed with the current version of SPIR-V spec and/or exising extensions? I mean we still may need new execution modes and decorations, right? |
This is correct. However, I think you should not compare the compiler FE with the SPIR-V spec. Instead, think of it this way. Let's say we want to add some new compile-time property in the future which also needs to be represented in SPIR-V. We would create a SYCL extension to describe the new property and we would also create a SPIR-V extension to describe the new matching decoration. Thus, we create an extension at both the SYCL level and also the SPIR-V level. It so happens that we can implement the SYCL extension in the header files without changing the front-end compiler. That is just an implementation detail, though. |
* When representing properties using `@llvm.ptr.annotation`, represent all properties in the fifth argument. This allows each property and its value to be represented as its own metadata, rather than combining them all into a single string. * Add an initial optional parameter to each C++ attribute that allows filtering of the properties. * The header now passes `nullptr` instead of `""` to represent the "value" of a property that has no value. * Clarify that each property in the C++ attribute parameter list has exactly one value, so the number of parameters is even (assuming the initial optional parameter is not specified).
gmlueck
dismissed stale reviews from romanovvlad and AlexeySachkov
via
4476369
* Capture some more open issues with properties that are represented as IR attributes on kernel arguments. What happens if an single aggregate kernel argument gets properties from more than one source? * Resolve TODO about replacing `handler::kernel_single_task()` with a wrapper class like `KernelSingleTaskWrapper`. The front-end team thinks this is the preferred direction. * Add note that `<initializer_list>` must be included in order to use the optional "filter list" parameter to the C++ attributes. This "filter list" parameter is a brace-enclosed list, and the front-end team thinks it would be easier to implement if `<initializer_list>` is included.
Solve the TODO issues with properties that decorate kernel parameter by: * Move the C++ attribute from the parameter's class to a member variable inside the class. The author of the header file will need to decide with member variable to attach the properties to. * Restrict the C++ attribute, so it is only used to decorate a SYCL "special class". When a value of this type is passed as a kernel parameter, each member variable is passed as a separate parameter to the kernel's function. As a result, there is no ambiguity about which function parameter receives the property.
gmlueck
requested review from
AaronBallman,
elizabethandrews,
GarveyJoe and
steffenlarsen
and removed request for
AlexeySotkin
|
Finalization of this design document is on the critical path for resuming implementation of this feature, so I'd like to get approvals from stakeholders. @elizabethandrews, @AaronBallman: Can one of you approve from the CFE team? I think all of the issues you raised earlier have been resolved. The only change since your last review is d0622a6, which eliminates one of the TODO items. @GarveyJoe: Since d0622a6 addressed your comment, can you approve also? @steffenlarsen: Adding you as an approver also, since you will be (and have been) implementing much of this design. Note that there are still two TODO's in this design about extending the set of property values that can be represented in IR. The design is workable without addressing those items, so I'd like to get approval in the current form. If we decide that we need to expand the set of legal IR properties later, we can update this design at that time. @bader: Please let me know if we need additional approvals before this can be merged. |
Clarify the description about the changes to the header file to state exactly which C++ attribute is added.
gmlueck
dismissed stale reviews from GarveyJoe and elizabethandrews
via
9ad4651
|
@bader: Can this be merged? In addition to the current approvers, the following people have previously approved earlier versions of this design. Their approvals were dismissed by later commits: Note that testing fails with the HIP backend on the following tests:
I think this must be unrelated since this PR changes only a document. In addition, the same three tests fail in #5268 |
Add a design document for the DPC++ extension API
SYCL_EXT_ONEAPI_PROPERTY_LIST, which describes how compile-timeproperties are recognized by the front-end, how they are represented
in LLVM IR, and how they are translated into SPIR-V.