-
Notifications
You must be signed in to change notification settings - Fork 555
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
spirv-diff reports spurious differences in OpFunctionParameter
instructions
#5218
Comments
Here's another case: ; SPIR-V
; Version: 1.6
; Generator: Khronos SPIR-V Tools Assembler; 0
; Bound: 109
; Schema: 0
OpCapability Shader
%1 = OpExtInstImport "GLSL.std.450"
OpMemoryModel Logical GLSL450
OpEntryPoint Fragment %36 "main_vec4vec3" %24 %26 %28 %30 %32 %34
- OpEntryPoint Fragment %85 "main_vec2scalar" %73 %75 %77 %79 %81 %83
+ OpEntryPoint Fragment %85 "main_vec2scalar" %73 %75 %77 %79 %81 %83
OpExecutionMode %36 OriginUpperLeft
OpExecutionMode %85 OriginUpperLeft
OpMemberDecorate %15 0 Offset 0
OpMemberDecorate %15 1 Offset 16 |
@ShabbyX PTAL |
For example, say I've also seriously considered if it's just better to run some optimization algorithm (like genetic algorithm or something) that matches instructions and get the minimum diff it can find instead of trying to be smart about it 🤷 All that said, if you are able to, you'll have a better time diffing if you can make sure debug info is retained. At the same time, I'll see what I can do about the unnamed variable matching; in the very least, if there's only one of a given type it's most likely a match. |
The second one looks like a bug, the instructions are identical! |
These are output files from the Naga shader translator's test suite. I could make Naga attach debug information to function parameters. However, I've seen this behavior with functions that take only one parameter. |
In the cases that are causing difficulty for me (spirv-diff is a wonderful tool, by the way!! Incredibly helpful), the diff would go away entirely if spirv-diff chose the simplest pairing of instructions, so my guess is that this is probably not about improving heuristics in ambiguous cases. |
Reduced test case. |
Addresses one case mentioned in KhronosGroup#5218.
- 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.
When a program's "correct" output isn't well-defined, it's tempting to assume bugs originate in some deep algorithmic problem, but in my experience, it's usually just run-of-the-mill bugs, so it's worth drilling down a bit. But to keep that criticism in context - spirv-diff is incredibly valuable to Naga developers for evaluating the effects of changes on our SPIR-V output. Without spirv-diff, it's extremely tempting to just gloss over the SPIR-V output tests when reviewing a PR, as long as the output for the other shading languages looks good. This tool makes it much easier to do a thorough job. I imagine the same is true for almost anyone generating SPIR-V. |
Indeed, and thanks a lot for jumping in and fixing the bugs.
Thank you, that's encouraging to hear. And you are right, we use it in maintaining ANGLE's SPIR-V generator (but unfortunately the tool was born after ANGLE's SPIR-V generator was pretty much done 😰) |
Addresses one case mentioned in KhronosGroup#5218.
- 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.
Addresses one case mentioned in #5218.
- 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.
- 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.
spirv-diff
seems to sometimes draw a distinction betweenOpFunctionParameter
instructions that seem equivalent to me, resulting in spurious differences.For example, given the two attached files, running
produces output that begins as follows:
It seems to me that
%19
and%50
are identical.spirv-diff-spurious.zip
The text was updated successfully, but these errors were encountered: