Relax diff labels rule for conditional jumps prior to SPIRV 1.6

[hernanponcedeleon]

The rule is only valid from SPIRV 1.6 onwards, thus we are currently rejecting perfectly valid kernels.

[github-actions]

Performance comparison

Linux x64

Benchmark details

Memory model: vmm

Benchmark	Base branch	PR branch	Improvement (99% CI)	Result
benchmarks/locks/cna.c	12.975 ± 0.182 s	12.855 ± 0.546 s	➖ +0.9% [-20.2%, +22.0%]	UNKNOWN
benchmarks/locks/mutex_musl.c	25.335 ± 2.448 s	21.168 ± 0.791 s	➖ +15.7% [-50.0%, +81.4%]	UNKNOWN
benchmarks/lfds/dglm.c	17.592 ± 1.156 s	17.612 ± 0.628 s	➖ -0.3% [-20.6%, +20.1%]	UNKNOWN
benchmarks/lfds/ms.c	29.455 ± 4.587 s	34.531 ± 0.976 s	➖ -18.7% [-98.7%, +61.3%]	UNKNOWN
benchmarks/lfds/treiber.c	9.242 ± 0.664 s	8.686 ± 0.232 s	➖ +5.8% [-28.5%, +40.0%]	UNKNOWN
benchmarks/lfds/safe_stack.c	6.949 ± 0.245 s	7.495 ± 0.225 s	➖ -7.9% [-26.8%, +11.0%]	UNKNOWN
benchmarks/challenging/cna.c	30.762 ± 0.610 s	32.632 ± 3.311 s	➖ -6.0% [-55.8%, +43.9%]	UNKNOWN

Memory model: aarch64

Benchmark	Base branch	PR branch	Improvement (99% CI)	Result
benchmarks/locks/linuxrwlock.c	6.931 ± 0.142 s	7.078 ± 0.193 s	➖ -2.1% [-8.5%, +4.3%]	UNKNOWN
benchmarks/challenging/cna.c	12.172 ± 0.322 s	12.942 ± 2.035 s	➖ -6.6% [-112.9%, +99.7%]	UNKNOWN
benchmarks/challenging/wsq.c	5.668 ± 0.055 s	5.597 ± 0.030 s	➖ +1.2% [-2.4%, +4.9%]	UNKNOWN

Memory model: power

Benchmark	Base branch	PR branch	Improvement (99% CI)	Result
benchmarks/locks/linuxrwlock.c	19.589 ± 0.322 s	19.693 ± 0.130 s	➖ -0.6% [-12.8%, +11.7%]	UNKNOWN
benchmarks/locks/mutex_musl.c	15.430 ± 0.790 s	15.236 ± 0.575 s	➖ +1.1% [-38.8%, +40.9%]	UNKNOWN
benchmarks/lfds/ms.c	18.757 ± 0.481 s	18.021 ± 0.313 s	➖ +3.9% [-14.3%, +22.0%]	UNKNOWN
benchmarks/lfds/treiber.c	14.379 ± 0.078 s	14.182 ± 0.458 s	➖ +1.4% [-14.2%, +17.0%]	UNKNOWN

Total

Benchmarks	Base branch	PR branch	Improvement (99% CI)
All reported benchmarks	225.235 ± 6.922 s	227.728 ± 2.617 s	➖ -1.1% [-12.6%, +10.3%]

4 benchmark(s) omitted because both averages were below 5 seconds.

macOS ARM64

Benchmark details

Memory model: vmm

Benchmark	Base branch	PR branch	Improvement (99% CI)	Result
benchmarks/locks/cna.c	23.150 ± 2.036 s	22.419 ± 0.760 s	➖ +2.6% [-56.8%, +62.0%]	UNKNOWN
benchmarks/locks/mutex_musl.c	29.294 ± 3.331 s	29.121 ± 1.762 s	➖ +0.1% [-38.3%, +38.6%]	UNKNOWN
benchmarks/lfds/dglm.c	54.258 ± 2.749 s	55.115 ± 4.150 s	➖ -1.6% [-29.8%, +26.7%]	UNKNOWN
benchmarks/lfds/ms.c	98.000 ± 8.185 s	85.333 ± 8.021 s	➖ +12.9% [-19.1%, +44.8%]	UNKNOWN
benchmarks/lfds/treiber.c	17.404 ± 0.436 s	16.614 ± 1.131 s	➖ +4.6% [-19.2%, +28.4%]	UNKNOWN
benchmarks/lfds/safe_stack.c	11.236 ± 0.417 s	14.323 ± 0.239 s	❌ -27.5% [-42.7%, -12.4%]	UNKNOWN
benchmarks/challenging/cna.c	45.992 ± 1.198 s	45.419 ± 0.677 s	➖ +1.2% [-10.9%, +13.4%]	UNKNOWN

Memory model: aarch64

Benchmark	Base branch	PR branch	Improvement (99% CI)	Result
benchmarks/locks/linuxrwlock.c	22.122 ± 0.848 s	21.893 ± 1.847 s	➖ +1.0% [-43.5%, +45.5%]	UNKNOWN
benchmarks/locks/mutex_musl.c	13.875 ± 0.923 s	13.559 ± 3.919 s	➖ +3.1% [-123.2%, +129.5%]	UNKNOWN
benchmarks/lfds/dglm.c	13.174 ± 1.208 s	11.733 ± 0.931 s	➖ +10.2% [-67.0%, +87.3%]	PASS
benchmarks/lfds/ms.c	15.275 ± 0.741 s	16.992 ± 0.833 s	❌ -11.2% [-12.8%, -9.7%]	UNKNOWN
benchmarks/challenging/cna.c	34.721 ± 3.438 s	32.224 ± 1.830 s	➖ +6.5% [-62.4%, +75.4%]	UNKNOWN
benchmarks/challenging/wsq.c	18.452 ± 0.896 s	17.372 ± 0.841 s	➖ +5.6% [-46.4%, +57.6%]	UNKNOWN

Memory model: power

Benchmark	Base branch	PR branch	Improvement (99% CI)	Result
benchmarks/locks/linuxrwlock.c	43.303 ± 13.518 s	49.652 ± 3.737 s	➖ -21.8% [-220.4%, +176.8%]	UNKNOWN
benchmarks/locks/mutex_musl.c	31.675 ± 5.516 s	29.617 ± 1.272 s	➖ +4.5% [-93.9%, +103.0%]	UNKNOWN
benchmarks/lfds/dglm.c	9.195 ± 0.790 s	10.358 ± 0.521 s	➖ -12.9% [-39.4%, +13.6%]	UNKNOWN
benchmarks/lfds/ms.c	59.793 ± 4.705 s	50.888 ± 1.081 s	➖ +14.6% [-13.8%, +43.1%]	UNKNOWN
benchmarks/lfds/treiber.c	25.654 ± 1.716 s	24.117 ± 2.052 s	➖ +5.8% [-46.9%, +58.4%]	UNKNOWN

Total

Benchmarks	Base branch	PR branch	Improvement (99% CI)
All reported benchmarks	566.572 ± 15.648 s	546.751 ± 11.406 s	➖ +3.4% [-17.1%, +24.0%]

[xeren]

LGTM

[hernanponcedeleon]

It seem spirv-val actually checks for this rule. E.g., the validation passes for this shader

  ; SPIR-V
  ; Version: 1.5
  ; Generator: Khronos SPIR-V Tools Assembler; 0
  ; Bound: 8
  ; Schema: 0
                 OpCapability Shader
                 OpMemoryModel Logical GLSL450
                 OpEntryPoint GLCompute %1 "main"
         %void = OpTypeVoid
         %bool = OpTypeBool
         %true = OpConstantTrue %bool
            %5 = OpTypeFunction %void
            %1 = OpFunction %void None %5
            %6 = OpLabel
                 OpBranchConditional %true %7 %7
            %7 = OpLabel
                 OpReturn
                 OpFunctionEnd

but fails for this one

  ; SPIR-V
  ; Version: 1.6
  ; Generator: Khronos SPIR-V Tools Assembler; 0
  ; Bound: 8
  ; Schema: 0
                 OpCapability Shader
                 OpMemoryModel Logical GLSL450
                 OpEntryPoint GLCompute %1 "main"
         %void = OpTypeVoid
         %bool = OpTypeBool
         %true = OpConstantTrue %bool
            %5 = OpTypeFunction %void
            %1 = OpFunction %void None %5
            %6 = OpLabel
                 OpBranchConditional %true %7 %7
            %7 = OpLabel
                 OpReturn
                 OpFunctionEnd

We should probably remove the check all-together and let spirv-val validate the kernels before we run them with dartagnan. I can add a new compilation pipeline rule which would do this for spvasm inputs.

[ThomasHaas]

I wanted to comment about this yesterday but held back on it. I was always under the impression that Dartagnan might be doing too many basic validity checks that should never fail unless the SPIRV code was handwritten as the SPIRV-generating frontend should be doing those checks already. I mean, having those (redundant?) checks is not bad but it takes quite a bit of effort to get them all right for practically little benefit.
In the case of this PR, the checks for wrong labels/jumps is not even semantically relevant, i.e., it seems like an arbitrary syntactic restriction (there is probably a legitimate reason for the restriction though).

[hernanponcedeleon]

was always under the impression that Dartagnan might be doing too many basic validity checks that should never fail unless the SPIRV code was handwritten as the SPIRV-generating frontend should be doing those checks already.

Writing SPIRV manually is a valid way of doing it (AFAIR Natalia has done this many times). Also, we should not rely on a generator having checks (I have seen generators getting it wrong). However, spirv-tools has a validator which should have most up-to-date rules in place, so the best way would be to use it.