Skip to content
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

Fix asserting from uvec2 buffer reference bitcast #259

Merged

Conversation

spencer-lunarg
Copy link
Contributor

closes #257

confirmed doing more research that we are safe to return 0 for a push constant that tries to use uvec2 to pass in a buffer reference handle

uint32_t base_id = ac->base_id;
// Access Chains mostly have their Base ID pointed directly to a OpVariable, but sometimes
// it will be through a load and this funciton handles the edge cases how to find that
static uint32_t FindAccessChainBaseVariable(SpvReflectPrvParser* p_parser, SpvReflectPrvAccessChain* p_access_chain) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

while here, updated this function name and ac to p_access_chain for consistency

@corporateshark
Copy link
Contributor

confirmed doing more research that we are safe to return 0 for a push constant that tries to use uvec2 to pass in a buffer reference handle

Based on this logic, will we obtain the correct byte size for push constants?

@spencer-lunarg
Copy link
Contributor Author

will we obtain the correct byte size for push constants?

Yes, as shown from the shader in the issue, the bufferId is still properly being accounted for (and properly marked as used)

          // size = 128, padded size = 128
          struct PerFrameData {               
              column_major float4x4 MVP;       // abs offset =   0, rel offset =   0, size = 64, padded size = 64 UNUSED
              uint2                 bufferId;  // abs offset =  64, rel offset =  64, size =  8, padded size =  8
              uint                  textureId; // abs offset =  72, rel offset =  72, size =  4, padded size =  4 UNUSED
              float                 time;      // abs offset =  76, rel offset =  76, size =  4, padded size =  4
              uint                  numU;      // abs offset =  80, rel offset =  80, size =  4, padded size =  4
              uint                  numV;      // abs offset =  84, rel offset =  84, size =  4, padded size =  4
              float                 minU;      // abs offset =  88, rel offset =  88, size =  4, padded size =  4 UNUSED
              float                 maxU;      // abs offset =  92, rel offset =  92, size =  4, padded size =  4 UNUSED
              float                 minV;      // abs offset =  96, rel offset =  96, size =  4, padded size =  4 UNUSED
              float                 maxV;      // abs offset = 100, rel offset = 100, size =  4, padded size =  4 UNUSED
              uint                  P1;        // abs offset = 104, rel offset = 104, size =  4, padded size =  4
              uint                  P2;        // abs offset = 108, rel offset = 108, size =  4, padded size =  4
              uint                  Q1;        // abs offset = 112, rel offset = 112, size =  4, padded size =  4
              uint                  Q2;        // abs offset = 116, rel offset = 116, size =  4, padded size =  4
              float                 morph;     // abs offset = 120, rel offset = 120, size =  4, padded size =  8
          } pc;  

The issue going on here was because inside a buffer reference struct, you can have things pointing to itself (ex, linked list)

layout(buffer_reference) buffer VertexBuffer {
    VertexBuffer next_pointer;
};

and we have logic to handle this recursion, but a Push Constants block, it can't be a buffer_reference block by definition, so there is no way to have it point to itself, so we don't need to chase down the buffer reference chain and can just return early

@corporateshark
Copy link
Contributor

Thanks @spencer-lunarg Looking forward to seeing it merged!

@spencer-lunarg spencer-lunarg merged commit b50ffbd into KhronosGroup:main Apr 9, 2024
5 checks passed
@spencer-lunarg spencer-lunarg deleted the spencer-lunarg-bitcast branch April 9, 2024 00:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error parsing compute shader SPIR-V generated by glslang 14.1.0.
3 participants