-
Notifications
You must be signed in to change notification settings - Fork 781
[SYCL] Add specialization constant feature design doc. #2572
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Signed-off-by: Konstantin S Bobrovsky <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Formatting/visualization comments inlined
Co-authored-by: Pavel Chupin <[email protected]>
Signed-off-by: Konstantin S Bobrovsky <[email protected]>
@AlexeySotkin , ping |
sycl/doc/SpecializationConstants.md
Outdated
%gold_POD = call %struct.POD __spirvCompositeSpecConstant<POD mangling>(10, 11, 12, 13, 14, 15) | ||
``` | ||
|
||
where `__spirvCompositeSpecConstant<type mangling>` is a new SPIR-V intrinsic which |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are at least 3 different ways to denote mangling of the POD type in this document. I believe they all mean the same thing, can we have single representation for it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK
sycl/doc/SpecializationConstants.md
Outdated
the primitive spec constants. | ||
- Then the translator will handle `__spirvCompositeSpecConstant*` intrinsic. It will | ||
recursively traverse the spec constant type structure in parallel with the argument | ||
list - which is a list of primitive spec constant operation IDs (not their SpecIds!). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit confused. What is the argument list? Is it the argument list of __spirvCompositeSpecConstant*
? But the example above says that it is a list of SpecId indeed. Having SpecIds here looks logical to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch - will change. It should be SpecIDs of course
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is the SPIRV OpSpecConstantComposite operation, where arguments (operands) are spec constant result IDs rather than SpecIDs
Co-authored-by: Alexey Sotkin <[email protected]>
Signed-off-by: Konstantin S Bobrovsky <[email protected]>
With AOT everything is simplified - the `SpecConstants` pass simply replaces | ||
the `__sycl_getSpecConstantValue` calls with constants - default values of | ||
the spec constant's type. No maps are generated, and SYCL program can't change | ||
the value of a spec constant. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it makes sense to mention what happens if we try to change value of the spec constant in AOT mode. I guess an exception is thrown in this case, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. But this is spec, I don't think we need to duplicate it here.
sycl/doc/SpecializationConstants.md
Outdated
And 1 "composite" | ||
|
||
```cpp | ||
%gold_POD = call %struct.POD __spirvCompositeSpecConstant<POD type mangling>(i32 10, i32 11, i32 12, i32 13, i32 14, i32 15) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is 15
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a typo, will remove
Co-authored-by: Alexey Sotkin <[email protected]>
Co-authored-by: Alexey Sotkin <[email protected]>
@AlexeySotkin please have a one more look before merge. I believe comments addressed? Pls. confirm. |
* sycl: [SYCL] Fix test failure due to size discrepancy in 32 and 64 bit environment (intel#2594) [BuildBot] Linux GPU driver uplift to 20.39.17972 (intel#2600) [SYCL][NFC] Remove cyclic dependency in headers (intel#2601) [SYCL][Doc] Fix Sphinx docs index (intel#2599) [SYCL][PI][L0] Add an assert to piEnqueueKernelLaunch() when the GlobalWorkOffset!=0 (intel#2593) [SYCL][Driver] Turn on -spirv-allow-extra-diexpressions option (intel#2578) [SYCL] Serialize queue related PI API in the Level-Zero plugin (intel#2588) Added missing math APIs for devicelib. (intel#2558) [SYCL][DOC] Fix path to FPGA device selector (intel#2563) [SYCL][CUDA] Add basic sub-group functionality (intel#2587) [SYCL] Add specialization constant feature design doc. (intel#2572) [SYCL] Expand release lit test to CPU and ACC (intel#2589) [SYCL] Add clang support for FPGA kernel attribute scheduler_target_fmax_mhz (intel#2511) [SYCL][ESIMD] Fixed compiler crash in LowerESIMDVecArg pass (intel#2556)
update GitHub Cache action to 4.2.0
update GitHub Cache action to 4.2.0
The document describes design of existing spec constants implementation for primitive types, and proposed design for POD types support.
Signed-off-by: Konstantin S Bobrovsky [email protected]