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.

Sign up for GitHub

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

Jump to bottom

spirv-diff probably reports spurious differences when pairing functions by type #5226

Open
jimblandy opened this issue May 22, 2023 · 0 comments · May be fixed by #5227
Open

spirv-diff probably reports spurious differences when pairing functions by type #5226

jimblandy opened this issue May 22, 2023 · 0 comments · May be fixed by #5227
Labels
component:diff Issues related to spirv-diff enhancement

Comments

@jimblandy
Copy link
Contributor

I don't have a test case for this yet, but the fix in #5225 almost certainly also needs to be applied two the two spots in MatchFunctions that call GroupIdsAndMatch with Differ::GroupIdsHelperGetTypeId, which is never the right thing to do, since it ignores the established src/dst mappings for types.

I have a fix for this, and if you prefer, I can just fold it into #5225. But the fix doesn't have any effect on test/diff/diff_files, meaning that (assuming that there is indeed a bug here - it seems clear) it doesn't have any test coverage. Rather than expand the scope of #5218, I filed this separate issue.
jimblandy added a commit to jimblandy/SPIRV-Tools that referenced this issue May 22, 2023
@jimblandy
Fix pairing of functions by type.
2fd1d5e 
Change `Differ::MatchFunctions` to use `GroupIdsAndMatchByMappedId`,
instead of calling `GroupIdsAndMatch` with
`Differ::GroupIdsHelperGetTypeId`, which is never correct: doing so
compares src and dst type ids directly, rather than consulting the
established mapping.

Fixes KhronosGroup#5226.
@jimblandy jimblandy linked a pull request May 22, 2023 that will close this issue
Draft
jimblandy added a commit to jimblandy/SPIRV-Tools that referenced this issue Jun 6, 2023
@jimblandy
Fix pairing of functions by type.
6226f04 
Change `Differ::MatchFunctions` to use `GroupIdsAndMatchByMappedId`,
instead of calling `GroupIdsAndMatch` with
`Differ::GroupIdsHelperGetTypeId`, which is never correct: doing so
compares src and dst type ids directly, rather than consulting the
established mapping.

Fixes KhronosGroup#5226.
@s-perron s-perron added component:diff Issues related to spirv-diff enhancement labels Jun 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
component:diff Issues related to spirv-diff enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix pairing of functions by type. jimblandy/SPIRV-Tools
2 participants
@jimblandy @s-perron