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

Deduplicate witness table while specializing generic functions #6363

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

jkwak-work
Copy link
Collaborator

@jkwak-work jkwak-work commented Feb 14, 2025

This commit fixes an issue that Slang sometimes emits duplicated DiffPair_XX structs when targeting HLSL.

This fix is required for upgrading DXC version from 1.7 to 1.9. With DXC 1.7, it had been fine even with the duplicated structs as long as their member variables are identical. But with DXC 1.9, it causes a compile error.

Closes #6364

@jkwak-work jkwak-work added the pr: non-breaking PRs without breaking changes label Feb 14, 2025
@jkwak-work jkwak-work self-assigned this Feb 14, 2025
@jkwak-work jkwak-work requested a review from a team as a code owner February 14, 2025 18:18
@jkwak-work jkwak-work force-pushed the fix/duplicated_struct_for_DiffPair branch from 30a393e to fa32e98 Compare February 14, 2025 18:26
@jkwak-work jkwak-work changed the title Fix/duplicated struct for diff pair Deduplicated structs for diff pair Feb 14, 2025
@jkwak-work
Copy link
Collaborator Author

jkwak-work commented Feb 14, 2025

I created an issue for this PR:

@jkwak-work jkwak-work force-pushed the fix/duplicated_struct_for_DiffPair branch from fa32e98 to 0ba6c28 Compare February 14, 2025 22:10
@jkwak-work jkwak-work changed the title Deduplicated structs for diff pair Deduplicate witness table while specializing generic functions Feb 14, 2025
@jkwak-work
Copy link
Collaborator Author

The latest change on this PR removes the duplicated witness table while specializing the generics.
This is the most preferred way to handle the issue.

@jkwak-work jkwak-work force-pushed the fix/duplicated_struct_for_DiffPair branch from 9cd57c5 to f9f9ea7 Compare February 15, 2025 01:21
@jkwak-work
Copy link
Collaborator Author

I made changes as suggested.

@jkwak-work jkwak-work enabled auto-merge (squash) February 15, 2025 14:34
@jkwak-work
Copy link
Collaborator Author

jkwak-work commented Feb 18, 2025

From a quick inspection with LLM, it appears that the decorations need to be handled properly.
In fact, they are not decorations but children whose op is "kIROp_WitnessTableEntry".
They are needed for how to handle dzero, dadd, dmul and such.

This commit is to experiment the idea of what would happen if we
deduplicate the witness table based only on its type and its first
operand.

It turned out that it causes a crash, because it doesn't carry the
decorations and childs properly. It means we need to also consider them
as well as its type and its first operand. But if we do, we cannot
deduplicate anything.

It seems that this is not the right place to apply the deduplication.
@jkwak-work jkwak-work force-pushed the fix/duplicated_struct_for_DiffPair branch from 31829d4 to 9dbbcdf Compare February 18, 2025 20:52
@jkwak-work
Copy link
Collaborator Author

It appears the the witness-table-entry as the children are same when the type and concrete type are same.
I left a comment on the source code for an extra information.

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.

Slang generates duplicated structs with identical members
2 participants