spirv-diff reports spurious differences in `OpFunctionParameter` instructions

[jimblandy]

spirv-diff seems to sometimes draw a distinction between OpFunctionParameter instructions that seem equivalent to me, resulting in spurious differences.

For example, given the two attached files, running

$ spirv-diff before.spvasm after.spvasm

produces output that begins as follows:

                OpCapability Shader
           %1 = OpExtInstImport "GLSL.std.450"
                OpMemoryModel Logical GLSL450
                OpEntryPoint GLCompute %48 "main"
                OpExecutionMode %48 LocalSize 1 1 1
           %2 = OpTypeVoid
           %4 = OpTypeBool
           %3 = OpConstantTrue %4
           %7 = OpTypeFunction %2
          %14 = OpTypePointer Function %4
          %15 = OpConstantNull %4
          %17 = OpConstantNull %4
          %21 = OpTypeFunction %2 %4
          %32 = OpConstantNull %4
          %34 = OpConstantNull %4
           %6 = OpFunction %2 None %7
           %5 = OpLabel
                OpBranch %8
           %8 = OpLabel
                OpBranch %9
           %9 = OpLabel
                OpLoopMerge %10 %12 None
                OpBranch %11
          %11 = OpLabel
                OpBranch %12
          %12 = OpLabel
                OpBranchConditional %3 %10 %9
          %10 = OpLabel
                OpReturn
                OpFunctionEnd
          %20 = OpFunction %2 None %21
-         %19 = OpFunctionParameter %4
+         %50 = OpFunctionParameter %4
          %18 = OpLabel
          %13 = OpVariable %14 Function %15
          %16 = OpVariable %14 Function %17
                OpBranch %22
          %22 = OpLabel
                OpBranch %23
          %23 = OpLabel
                OpLoopMerge %24 %26 None
                OpBranch %25
          %25 = OpLabel
                OpBranch %26
          %26 = OpLabel
-               OpStore %13 %19
+               OpStore %13 %50
          %27 = OpLoad %4 %13
-         %28 = OpLogicalNotEqual %4 %19 %27
+         %28 = OpLogicalNotEqual %4 %50 %27
                OpStore %16 %28
          %29 = OpLoad %4 %16
-         %30 = OpLogicalEqual %4 %19 %29
+         %30 = OpLogicalEqual %4 %50 %29
                OpBranchConditional %30 %24 %23
          %24 = OpLabel
                OpReturn
                OpFunctionEnd

It seems to me that %19 and %50 are identical.

spirv-diff-spurious.zip

[jimblandy]

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

spirv-diff-another.zip

[alan-baker]

@ShabbyX PTAL

[ShabbyX]

spirv-diff heavily relies on debug information to match variables. It's obvious in this case that these two variables should have been matched, but things quickly get ambiguous when there are multiple variables.

For example, say f(float a, float b, float c) is being matched with f(float d, float e), i.e. the function has one parameter reduced. How do you match these variables? One possibility is to brute-force all possibilities and see which diff is the smallest, which might just be fast enough in practice.

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 :shrug:

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.

[ShabbyX]

The second one looks like a bug, the instructions are identical!

[jimblandy]

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.

[jimblandy]

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.

[jimblandy]

Reduced test case. spirv-diff-another.zip

[jimblandy]

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.

[ShabbyX]

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.

Indeed, and thanks a lot for jumping in and fixing the bugs.

spirv-diff is incredibly valuable to Naga developers for evaluating the effects of changes on our SPIR-V output ... I imagine the same is true for almost anyone generating SPIR-V

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 :cold_sweat:)