[GitHub] [tvm] Lunderberg commented on pull request #8102: [Vulkan] Remove some interface block decoration

[GitBox <gi...@apache.org>]

Lunderberg commented on pull request #8102:
URL: https://github.com/apache/tvm/pull/8102#issuecomment-846041537


   One suggestion, I think we should need to apply the `DecorationArrayStride` only when `interface_buffer` is false  (see below).  Everything else good to me.  Fantastic debugging, and thank you very much!
   
   
   
   General notes from reading over changes, and re-reading documentation for similar issues:
   
   * Do issues arise if the same struct is declared both as an interface and as a non-interface?  No, since you added the `interface_block` to the cache lookup, they would be treated as separate types.
   
   * Are there any other uses of `DecorationBlock` or `DecorationBufferBlock` that should be removed?  No, the only other use is in `DeclareStorageType`, which is only used when declaring I/O by push constants or uniform buffers, so it is fine.  That said, in the future we might want to rename `DeclareStorageType` to `DeclareIOStruct`, or add the same `interface_block` argument to it, to avoid implying that it could be used to declare a struct that isn't used for IO.
   
   * Are there any other decorations that should only be used conditionally?  Yes, `DecorationArrayStride`.
     * DecorationArrayStride - Looks like if an array contains a structure decorated by `Buffer` or `BufferBlock`, then the `ArrayStride` decoration must be omitted ([ref](https://www.khronos.org/registry/spir-v/specs/unified1/SPIRV.html#_a_id_shadervalidation_a_validation_rules_for_shader_a_href_capability_capabilities_a)).  Therefore, in `GetStructArrayType`, the `DecorationArrayStride` and  should only be applied if `interface_block` is false.
     * DecorationOffset - Should be used for each structure member, whether or not it is an interface type, no issues.
     * DecorationDescriptorSet, DecorationBinding - Used for buffer inputs/outputs, no issues.
     * DecorationBuiltIn - Used for accessing WorkgroupId and LocalInvocationId, no issues.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org