Fix structured exit validation

[alan-baker]

Fixes #3139

• If the header of the construct is also a merge block, jump to the
  associated header instead of the immediate dominator
• new test

[paulthomson]

Looks good. I think I get it now. The code for checking the exits from a selection:

• Is happy if the exit is to the merge of the selection
• Otherwise, it traverses the immediate dominator chain, looking for headers and checking if the exit is to one of the merges. It also tracks whether we have seen a switch, in which case breaking to a loop merge is no longer a valid exit. The test hits this last case incorrectly.

The test's selection construct's header is also a loop's merge block. But actually, I don't think this is the main issue. The main problem is that the immediate dominator of the selection header / loop merge is not the loop header, as you might expect, but the continue target, and this then leads us into the loop construct, instead of working our way through the outer constructs as we probably expect. The immediate dominator is a selection switch header. This does not help validate the exit, so we move on again, but we incorrectly track the fact that we have seen a switch in the outer constructs (but the switch is not actually an outer construct); the switch's merge block unexpectedly does not dominate the selection construct's header, which would otherwise prevent this. When we get to the relevant outer switch, it is too late because it seems like the exit is escaping an inner switch, so validation of the exit fails.

I think the fix only address the first block in the traversal. I believe the same issue could occur at any point in the traversal. Below is a test that I think fails, even with the fix. There may be more issues we could expose via the merge block -> continue target traversal. I wonder if this is the real issue. We are essentially traversing into a construct, which I guess is not what we want when traversing immediate dominators. Perhaps when we encounter a merge block, we should always then move to the associated header? And otherwise, go to the immediate dominator. The check for if we are at a merge block could be the first thing we do in the loop, so the first block in the traversal can simply remain as the header.

Test that I think fails:

; SPIR-V
; Version: 1.0
; Generator: Khronos Glslang Reference Front End; 8
; Bound: 47
; Schema: 0
               OpCapability Shader
          %1 = OpExtInstImport "GLSL.std.450"
               OpMemoryModel Logical GLSL450
               OpEntryPoint Fragment %4 "main"
               OpExecutionMode %4 OriginUpperLeft
               OpSource ESSL 310
               OpName %4 "main"
               OpName %8 "i"
               OpDecorate %8 RelaxedPrecision
               OpDecorate %10 RelaxedPrecision
               OpDecorate %18 RelaxedPrecision
               OpDecorate %20 RelaxedPrecision
               OpDecorate %21 RelaxedPrecision
               OpDecorate %28 RelaxedPrecision
               OpDecorate %29 RelaxedPrecision
               OpDecorate %31 RelaxedPrecision
               OpDecorate %35 RelaxedPrecision
               OpDecorate %40 RelaxedPrecision
               OpDecorate %41 RelaxedPrecision
               OpDecorate %44 RelaxedPrecision
               OpDecorate %45 RelaxedPrecision
          %2 = OpTypeVoid
          %3 = OpTypeFunction %2
          %6 = OpTypeInt 32 1
          %7 = OpTypePointer Function %6
          %9 = OpConstant %6 0
         %19 = OpConstant %6 1
         %32 = OpConstant %6 100
         %33 = OpTypeBool
         %51 = OpConstantTrue %33
         %39 = OpConstant %6 2
         %43 = OpConstant %6 3
          %4 = OpFunction %2 None %3

          %5 = OpLabel
          %8 = OpVariable %7 Function
               OpStore %8 %9
         %10 = OpLoad %6 %8
               OpSelectionMerge %13 None
               OpSwitch %10 %12 0 %11

            %12 = OpLabel
            %44 = OpLoad %6 %8
            %45 = OpISub %6 %44 %43
                OpStore %8 %45
                OpBranch %13

            %11 = OpLabel
                OpBranch %14

            %14 = OpLabel
                OpLoopMerge %16 %17 None
                OpBranch %15

                %15 = OpLabel
                %18 = OpLoad %6 %8
                %20 = OpIAdd %6 %18 %19
                    OpStore %8 %20
                %21 = OpLoad %6 %8
                    OpSelectionMerge %25 None
                    OpSwitch %21 %24 1 %22 2 %23

                    %24 = OpLabel
                    %28 = OpLoad %6 %8
                    %29 = OpISub %6 %28 %19
                        OpStore %8 %29
                        OpBranch %25

                    %22 = OpLabel
                        OpBranch %17

                    %23 = OpLabel
                        OpBranch %25

                %25 = OpLabel
                    OpBranch %17

                %17 = OpLabel
                %31 = OpLoad %6 %8
                %34 = OpSGreaterThan %33 %31 %32
                    OpBranchConditional %34 %14 %16

            %16 = OpLabel
            %35 = OpLoad %6 %8
            %36 = OpSGreaterThan %33 %35 %32
                OpSelectionMerge %38 None
                OpBranchConditional %36 %37 %38

                %37 = OpLabel
                %40 = OpLoad %6 %8
                %41 = OpISub %6 %40 %39
                    OpStore %8 %41
                    OpSelectionMerge %50 None
                    OpBranchConditional %51 %52 %50

                    %52 = OpLabel
                    OpBranch %13

                
                %50 = OpLabel
                OpBranch %38

            %38 = OpLabel
                OpBranch %12

         %13 = OpLabel
               OpReturn
               OpFunctionEnd

[paulthomson]

Here is another potentially interesting test case that I think still fails:

; SPIR-V
; Version: 1.0
; Generator: Khronos Glslang Reference Front End; 7
; Bound: 27
; Schema: 0
               OpCapability Shader
          %1 = OpExtInstImport "GLSL.std.450"
               OpMemoryModel Logical GLSL450
               OpEntryPoint Fragment %4 "main"
               OpExecutionMode %4 OriginUpperLeft
               OpSource ESSL 310
               OpName %4 "main"
          %2 = OpTypeVoid
          %3 = OpTypeFunction %2
          %6 = OpTypeInt 32 1
          %7 = OpConstant %6 1
         %15 = OpTypeBool
         %16 = OpConstantTrue %15
         %17 = OpConstant %6 2
          %4 = OpFunction %2 None %3

          %5 = OpLabel
               OpSelectionMerge %9 None
               OpSwitch %7 %9 1 %8

            %8 = OpLabel
                  OpBranch %10

            %10 = OpLabel
                  OpLoopMerge %12 %13 None
                  OpBranch %11

                  %11 = OpLabel
                        OpSelectionMerge %20 None
                        OpSwitch %17 %19 2 %18

                        %19 = OpLabel
                              OpBranch %20

                        %18 = OpLabel
                              OpBranch %13

                  %20 = OpLabel
                        OpBranch %12

                  %13 = OpLabel
                        OpBranchConditional %16 %12 %10

            %12 = OpLabel
                  OpSelectionMerge %24 None
                  OpBranchConditional %16 %23 %24

                  %23 = OpLabel
                        OpBranch %9

            %24 = OpLabel
                  OpBranch %9

          %9 = OpLabel
               OpReturn
               OpFunctionEnd

[alan-baker]

Thanks for the example. The first failed and second passed. I think I understand what you're saying. I will change the basic traversal logic to use the same logic as the first block for all transitions. That fixes the first example.

[paulthomson]

LGTM! Yes, I somehow overlooked the fact that my suggestion was exactly what you had already done, except not just for the first block!