Add support for SPV_KHR_constant_data and SPV_KHR_abort#4196
Add support for SPV_KHR_constant_data and SPV_KHR_abort#4196Samsung-CraigG wants to merge 10 commits intoKhronosGroup:mainfrom
Conversation
|
Zhou, Shaochi(AMD) seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
|
Mirrors gitlab PR#141. |
Samsung-CraigG
changed the title
|
@Samsung-CraigG You're going to need to fix the CI failure before this can be merged. |
|
@arcady-lunarg done (sorry, hadn't noticed as Shaochi @ AMD wrote the code & I'd not tried a GCC build) |
|
@Samsung-CraigG Can you ask the other authors to sign the CLA. |
|
Hi, this is Shaochi, and this is my account for open-source commit submission. |
|
I've just been discussing this with Ralph (as there seems to be a process issue here). If @ShchchowAMD has time to squish & re-submit this PR that'd be great for this instance. (we can discuss internally at Khronos ways to avoid this next time around) |
|
I don't think you need to create a new PR but you do need to create a new branch with commits that have the appropriate author emails. The CLA bot ultimately goes off the emails in the author field in the commits. |
- with no instructions following in the same way as OpReturn.
|
Sorry for the late response. I'll create commits with this account this weekend for abortKHR. |
|
Thanks @ShchchowAMD - appreciated. |
|
Hi, I've created a new commit for submission under branch: Please feel free to tell me what extra works needed if anything works not as expected, sorry for blocking PR submission. |
| const char* formatSpecifiers[formatSpecifiersSize] = {"%d", "%i", "%f", "%u"}; | ||
| // 1. Split original message string with format specifiers. | ||
| const glslang::TString* msg = !isEmptyMsg ? glslangOperands[0]->getAsConstantUnion()->getConstArray()[0].getSConst() | ||
| : new glslang::TString("\0"); |
There was a problem hiding this comment.
msg will own the heap allocation if the message is empty. This will likely create leaks. Could you either allocate on the stack or restructure the branches so no heap allocation is needed?
| constStrInfo(glslang::TString str, int spec) : string(str), specifierIndex(spec){}; | ||
| } strInfo; | ||
|
|
||
| std::vector<strInfo> splitedStr, tempSplitedStr; |
There was a problem hiding this comment.
grammar nit: s/splitedStr/splitStr/ and s/tempSplitedStr/tempSplitStr/
| pos = str.find(formatSpecifiers[i]); | ||
| } | ||
| if (str.size() > 0 || isEmptyMsg) | ||
| tempSplitedStr.push_back(strInfo(str, specifierIndex)); |
There was a problem hiding this comment.
nit: Check for !str.empty() instead of str.size() > 0.
Note that isEmptyMsg is set when glslangOperands is empty (no arguments). If the caller passes an empty string (e.g., abortEXT("", v);) then msg will have size 0. This will make the loop not push anything into tempSplitedStr. When it goes on to pop from structMemberOffsets, it will leave the vector empty. So when the code tries to access structMemberOffsets.back() the vector is empty (undefined behaviour).
This needs a test for an empty string and special handling for empty string literals ("").
There was a problem hiding this comment.
Yea. I'm now treating it as an empty string.
| "#define GL_EXT_shared_memory_block 1\n" | ||
| "#define GL_EXT_shader_integer_mix 1\n" | ||
| "#define GL_EXT_spec_constant_composites 1\n" | ||
| "#define E_GL_EXT_abort 1\n" |
There was a problem hiding this comment.
s/E_GL_EXT_abort/GL_EXT_abort/ since E_GL_EXT_abort is the C++ constant in Versions.h. The GLSL injection should use the GLSL extension name.
| Instruction* op = new Instruction(getUniqueId(), typeId, opCode); | ||
| op->reserveOperands(operands.size()); | ||
| for (auto id : operands) | ||
| op->addStringOperand(id); |
There was a problem hiding this comment.
Does the encoding use LiteralString or LiteralBytes?
There was a problem hiding this comment.
It is 8 bit width elements here, so should be Bytes I think?
https://gitlab.khronos.org/spirv/spirv-extensions/-/blob/b75fc40c686119e6590d1eabfabe2f843dc30e3a/SPV_KHR_constant_data.asciidoc
| builder.addExtension(spv::E_SPV_KHR_constant_data); | ||
| builder.addExtension(spv::E_SPV_KHR_abort); | ||
|
|
||
| typedef struct constStrInfo { |
There was a problem hiding this comment.
nit. No typedef necessary. This can use strInfo or constStrInfo. No need to have both.
| // 1. Split original message string with format specifiers. | ||
| const glslang::TString* msg = !isEmptyMsg ? glslangOperands[0]->getAsConstantUnion()->getConstArray()[0].getSConst() | ||
| : new glslang::TString("\0"); | ||
| splitedStr.push_back(strInfo(*const_cast<glslang::TString*>(msg), -1)); |
There was a problem hiding this comment.
No need to const_cast here. The constructor takes str by value. This can be strInfo(*msg, -1).
|
|
||
| void TGlslangToSpvTraverser::createAbortEXT(const glslang::TIntermSequence glslangOperands) | ||
| { | ||
| bool isEmptyMsg = glslangOperands.size() == 0; |
| return builder.createCompositeConstruct(resultTypeId, constituents); | ||
| } | ||
|
|
||
| void TGlslangToSpvTraverser::createAbortEXT(const glslang::TIntermSequence glslangOperands) |
There was a problem hiding this comment.
this will copy the vector on every call. Pass it by const reference, please.
Use builder.makeStatementTerminator() instead.
|
Closed as superseded by #4222 |
7 participants
Used by VK_KHR_shader_abort / VK_KHR_shader_constant_data