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 generic witness table SequentialIDDecorations #6129

Merged

Conversation

juliusikkala
Copy link
Contributor

@juliusikkala juliusikkala commented Jan 18, 2025

Fixes (hopefully) #5407, although I couldn't get OPs code compiling with the current master branch version of Slang. At least this PR fixes the two similar cases from the comments. This is likely not the correct way to do it, but it's probably easiest to discuss this in a PR since I want to fix this in any case.

The problem appears to be that witness tables for generic structs don't get SequentialIDDecorations. This is because ensureWitnessTableSequentialIDs checks for LinkageDecoration, which those generic witness tables don't have (but should they?). When addLinkageDecoration is called for witness tables in visitInheritanceDecl (slang-lower-to-ir.cpp), it starts by finding the outermost generic and attaches the decoration to that, which in this case is not the witness table itself.

I tried adding a new flag to addLinkageDecoration to skip that outermost-generic-search, skipping it only in visitInheritanceDecl, but that broke some tests, so it's probably not that simple.

So the overkill approach in this PR is to give SequentialIDDecoration to every single witness table for which ensureWitnessTableSequentialIDs is called. It doesn't seem to break any tests and fixes the code examples from that issue, but I guess there must be some negative consequences since clearly some effort was seen to only add the IDs in a very specific corner case.

@juliusikkala juliusikkala requested a review from a team as a code owner January 18, 2025 23:01
@juliusikkala juliusikkala changed the title Fix generic witness table IDs Fix generic witness table SequentialIDDecorations Jan 18, 2025
@csyonghe
Copy link
Collaborator

I can no longer recall why we need to skip assigning IDs for witness tables without a linkage. I don't see the downside of assigning every witness table a sequential ID. If all tests are passing, then this is fine and we will deal with any consequences if we find one in the future.

At this point all witness tables should have a linkage decoration due to the way we lower them to IR. And really the specialized generic witness tables are the only ones that do not have a linkage decoration but they shouldn't be excluded. So removing this logic seems the correct approach.

@csyonghe csyonghe added the pr: non-breaking PRs without breaking changes label Jan 20, 2025
@csyonghe csyonghe merged commit 6812245 into shader-slang:master Jan 21, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: non-breaking PRs without breaking changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants