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 pairing of function parameters. #5225

Merged

Conversation

jimblandy
Copy link
Contributor

  • Consider prior type pairings when attempting to pair function parameters by type.
  • Pair all parameters that have matching types, not just the first.
  • Update diff tests.

Fixes #5218.

@jimblandy
Copy link
Contributor Author

Reviewing the changes to those test files was a bear. Maybe someone should write a program that helps one ignore extraneous differences in... never mind.

@jimblandy
Copy link
Contributor Author

@ShabbyX If you could approve the CI run for this PR, then I can address whatever problems it shows up.

@s-perron s-perron added component:diff Issues related to spirv-diff kokoro:run labels May 23, 2023
@s-perron
Copy link
Collaborator

The CI-macos-clang-release-bazel and CI-shaderc-smoketest are not your problem. Please run your change through clang format. You can use git clang-format if you are not sure how to do that.

source/diff/diff.cpp Outdated Show resolved Hide resolved
source/diff/diff.cpp Outdated Show resolved Hide resolved
source/diff/diff.cpp Outdated Show resolved Hide resolved
@ShabbyX
Copy link
Contributor

ShabbyX commented May 24, 2023

Not sure if you missed a push, I don't see any diff w.r.t the resolved comments 🤔

@jimblandy
Copy link
Contributor Author

Sorry, that was just the formatting, I'm working on the comments right now. (Thought I'd resolved them, and then wanted to tweak them a bit more.)

- Consider prior type pairings when attempting to pair function
parameters by type.
- Pair all parameters that have matching types, not just the first.
- Update diff tests.

Fixes KhronosGroup#5218.
@jimblandy jimblandy force-pushed the use-type-pairings-for-parameters branch from 9d5cd69 to 6b50a18 Compare May 24, 2023 20:23
@jimblandy
Copy link
Contributor Author

Okay, I think 6b50a18 should address all comments.

@jimblandy
Copy link
Contributor Author

There's a general didactic principle, "concrete before abstract", so I changed the comment to start with the concrete example of pairing buckets by type. It's supposed to be easier to follow the example when holding that specific use case in mind.

Then I used the verb 'pair' for what one does with buckets, and 'match' for what one does with ids.

@ShabbyX
Copy link
Contributor

ShabbyX commented May 24, 2023

Nice, thanks.

Eventually, it would be nice to have the id "namespace" (src or dst) find its way to the type (as opposed to just uint32_t) such that the compiler would make sure src and dst ids aren't compared directly. Like a SrcId and DstId type that are distinct to the compiler (although identical)

@jimblandy
Copy link
Contributor Author

@s-perron This is ready to merge.

@s-perron s-perron merged commit 9ed2ac2 into KhronosGroup:main May 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:diff Issues related to spirv-diff
Projects
None yet
Development

Successfully merging this pull request may close these issues.

spirv-diff reports spurious differences in OpFunctionParameter instructions
4 participants