[spirv] Add support for [[vk::shader_record_nv]]#2179
Conversation
|
✅ Build DirectXShaderCompiler 1.0.1854 completed (commit 5888fba98a by @alelenv) |
ehsannas
changed the title
|
Great! Thanks @alelenv . I'll take a look. |
ehsannas
left a comment
ehsannas
left a comment
There was a problem hiding this comment.
Nice job! So happy to see documentation and tests! Looks good to me overall; Just a couple of nits pointed out below.
| astDecls[decl] = DeclSpirvInfo(var); | ||
|
|
||
| // Do not push this variable into resourceVars since it does not need | ||
| // descriptor set. |
There was a problem hiding this comment.
Thanks for the comment here
There was a problem hiding this comment.
I think you should also add
// CHECK-NOT: OpDecoration %bla DescriptorSet
// CHECK-NOT: OpDecoration %bla Binding
to your tests?
There was a problem hiding this comment.
Yes, this is a good idea. Added to tests
|
|
||
| // We still register all VarDecls seperately here. All the VarDecls are | ||
| // mapped to the <result-id> of the buffer object, which means when querying | ||
| // querying the <result-id> for a certain VarDecl, we need to do an extra |
There was a problem hiding this comment.
Typo: 'querying' repeated ;)
| } | ||
| if (decl->hasAttr<VKBindingAttr>()) { | ||
| emitError("vk::shader_record_nv attribute cannot be used together with " | ||
| "vk::binding attribute", |
There was a problem hiding this comment.
Can we add a test to make sure this error is emitted when needed?
There was a problem hiding this comment.
Added test to vk.attribute.invalid.hlsl
|
Thanks for quick turnaround of review |
|
✅ Build DirectXShaderCompiler 1.0.1875 completed (commit 6dc71346a8 by @alelenv) |
3 participants
@ehsannas
Please review fix for issue #2019
Tests have been added along with documentation update.
Thanks