[Mesa-dev] [PATCH] nir/spirv: short-circuit when conditional branch contains end block

[Juan A. Suarez Romero]

On Thu, 2019-03-07 at 07:15 -0600, Jason Ekstrand wrote:
> Woah, is this legal SPIR-V? I think a second OpSelectionMerge is required.

I'd say it is legal. The spec does not mandate that each branch has its own
merge instruction; only that the control flow must be structured for shaders.

In section 2.11 ("Structured Control Flow"), about structured control-flow
constructs:


- A structured control-flow construct is then defined as one of:
  - a selection construct: the set of blocks dominated by a selection header,
minus the set of blocks dominated by the header’s merge block
  - [...]

- The above structured control-flow constructs must satisfy the following rules:
  - [...]
  - the only blocks in a construct that can branch outside the construct are
    - a block branching to the construct’s merge block
    - a block branching from one case construct to another, for the same
OpSwitch
    - a back-edge block
    - a continue block for the innermost loop it is nested inside of
    - a break block for the innermost loop it is nested inside of
    - a return block


Our selection construct, which contains the two nested conditional branches,
satisfies the rules, as both conditionals branches to the construct's merge
block.


	J.A.






> 


> --Jason
> 
> 
> On March 6, 2019 05:25:26 "Juan A. Suarez Romero" <jasuarez at igalia.com> wrote:
> 
> > This fixes the case when the SPIR-V code has two nested conditional
> > branches, but only one selection merge:
> > 
> > 
> > [...]
> > %1 = OpLabel
> >     OpSelectionMerge %2 None
> >     OpBranchConditional %3 %4 %2
> > %4 = OpLabel
> >     OpBranchConditional %3 %5 %2
> > %5 = OpLabel
> >     OpBranch %2
> > %2 = OpLabel
> > [...]
> > 
> > 
> > In the second OpBranchConditional, as the else-part is the end
> > block (started in the first OpBranchConditional) we can just follow the
> > then-part.
> > 
> > 
> > This fixes dEQP-VK.vkrunner.controlflow.2-obc-triangle-triangle
> > 
> > 
> > CC: Jason Ekstrand <jason at jlekstrand.net>
> > ---
> > src/compiler/spirv/vtn_cfg.c | 11 ++++++++++-
> > 1 file changed, 10 insertions(+), 1 deletion(-)
> > 
> > 
> > diff --git a/src/compiler/spirv/vtn_cfg.c b/src/compiler/spirv/vtn_cfg.c
> > index 7868eeb60bc..f749118efbe 100644
> > --- a/src/compiler/spirv/vtn_cfg.c
> > +++ b/src/compiler/spirv/vtn_cfg.c
> > @@ -605,7 +605,16 @@ vtn_cfg_walk_blocks(struct vtn_builder *b, struct 
> > list_head *cf_list,
> >             }
> >          } else if (if_stmt->then_type == vtn_branch_type_none &&
> >                     if_stmt->else_type == vtn_branch_type_none) {
> > -            /* Neither side of the if is something we can short-circuit. */
> > +            /* Neither side of the if is something we can short-circuit,
> > +             * unless one of the blocks is the end block. */
> > +            if (then_block == end) {
> > +               block = else_block;
> > +               continue;
> > +            } else if (else_block == end) {
> > +               block = then_block;
> > +               continue;
> > +            }
> > +
> >             vtn_assert((*block->merge & SpvOpCodeMask) == SpvOpSelectionMerge);
> >             struct vtn_block *merge_block =
> >                vtn_value(b, block->merge[1], vtn_value_type_block)->block;
> > --
> > 2.20.1
> 
> 
>