spirv-val: Conditional Branch without an exit is invalid in loop header

[cmarcelo]

From 2.16.2, for CFG:

Selections must be structured. That is, an OpSelectionMerge instruction is required to precede:

    an OpSwitch instruction

    an OpBranchConditional instruction that has different True Label and False Label operands where neither are declared merge blocks or Continue Targets.

[gfxstrand]

I don't have approval permissions but this looks correct to me.

[dneto0]

The Tint SPIRV-Reader currently handles this case.
When I modify this case to add an Fragment entry point, and remove Linkage, I get this:

$ tint --format wgsl b.spvasm
fn main_1() {
  loop {
    if (false) {
      continue;
    } else {
      break;
    }
  }
  return;
}

@fragment
fn main() {
  main_1();
}

Yeah, I went the extra mile to handle this extra case.
Not passing judgement on this PR. Just FYI.

[alan-baker]

I agree with your interpretation of the spec, but I don't remember discussing this specific case in Khronos internal SPIR issue 115. I'm attempting to double check the Alloy model that was recently developed to see what was tested there.

[gfxstrand]

I went back and did some digging and sadly neither the issue nor the meeting minutes were particularly informative. 😩 I do remember there being a discussion and that I pushed for the current rules. I don't remember what all else was said and I didn't bother to go dig up a recording.

[alan-baker]

@cmarcelo have you tested this against CTS by chance? I assume it's ok, but I know LLVM would like to generate code like this sometimes.

[gfxstrand]

Yes, we tested it against the CTS as well as a large pile of additional tests from the fuzzing folks.

As far as LLVM goes, you can get exactly the same thing by translating:

OpLoopMerge %merge, %continue
OpBranchConditional %a, %b

with

OpLoopMerge %merge, %continue
OpBranch %c
%c = OpLabel
OpSelectionMerge %continue
OpBranchConditional %a, %b

and let the early merge logic take care of the rest. It's exactly 3 more SPIR-V instructions. I don't think this will hurt LLVM's ability to generate SPIR-V. The difference is that we now explicitly have two different constructs: a selection and a loop, instead of a loop that's also kind-of a free selection.

[cmarcelo]

CC: @Jack-Clark

[alan-baker]

The work on the alloy model hasn't revealed any surprising cases that are disallowed by this change (nor was it expected to). The change looks good and workarounds in generators are reasonable.

[alan-baker]

I believe the VS2019 failures are due to this being an older branch. Could you please rebase this PR?

[cmarcelo]

Rebased.

[alan-baker]

It appears that several unit tests are broken by this change.

[cmarcelo]

That's odd... I'll check what's up.

[cmarcelo]

@alan-baker is there a way to see the details of the "CI-ubuntu-clang" runs?

[cmarcelo]

Nevermind, managed to load other runs results here. The issue seems to be with fuzzer, I'll look into that.

[alan-baker]

When the message is "Kokoro build finished" it generally indicates an internal error in the CI process. I try to retrigger those builds when I see them.

[cmarcelo]

@alan-baker there were a few Fuzz internal tests that were assuming an initial state invalid. I've added a separate patch to this PR to fix them. Could you trigger the builds again?

[cmarcelo]

For the record: I've tested by enabling -DSPIRV_BUILD_FUZZER=ON (that's not the default, which is why I've missed before).