Skip to content
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-val: Add test for Conditional Branch without an exit being valid #5070

Conversation

cmarcelo
Copy link
Contributor

@cmarcelo cmarcelo commented Jan 18, 2023

This patch is mutual exclusive with #5069.

@Keenuts
Copy link
Contributor

Keenuts commented Jan 19, 2023

Not a SCFG/CFG expert at all, but I'd say this should compile?

  • I'm assuming Undef doesn't mean a block is unreachable, just we have an undefined value, so branch decision will be implementation defined.
  • From the SPIR-V spec around SCGF, it seems the structure is OK, and we are not breaking rules around loop structure, as we have an exit which seem structurally OK.

@cmarcelo
Copy link
Contributor Author

Not a SCFG/CFG expert at all, but I'd say this should compile?

* I'm assuming Undef doesn't mean a block is unreachable, just we have an undefined value, so branch decision will be implementation defined.

* From the SPIR-V spec around SCGF, it seems the structure is OK, and we are not breaking rules around loop structure, as we have an exit which seem structurally OK.

I was unsure whether the OpBranchConditional would require an OpSelectionMerge in this case. Upon discussing with others it seems this is covered by:

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.

so would make this invalid SPIR-V.

I'm closing this in favor of #5069.

@cmarcelo cmarcelo closed this Jan 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants