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

[Jason Ekstrand]

On Fri, Mar 8, 2019 at 9:30 AM Juan A. Suarez Romero <jasuarez at igalia.com>
wrote:

> 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.
>

That's exactly the problem.  It says how merge instructions work and how
they have to be nested but never that or where you have to use them. :-(
This is a spec bug.  The closest I can find is this line from the
structured control flow section: "These explicitly declare a header block
<https://www.khronos.org/registry/spir-v/specs/unified1/SPIRV.html#HeaderBlock>
before the control flow diverges and a merge block
<https://www.khronos.org/registry/spir-v/specs/unified1/SPIRV.html#MergeBlock>
where control flow subsequently converges."  If you read that as "you must
declare divergence" then it would imply that every OpBranchConditional must
be preceded by an OpSelectionMerge.  If we don't require this, there are
lots of weird cases where you can get into diamond patters and other things
that aren't trivially structurizable.

--Jason


> 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
> >
> >
> >
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20190308/67c43a19/attachment-0001.html>