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

[Jason Ekstrand]

On Thu, Mar 14, 2019 at 4:09 AM Juan A. Suarez Romero <jasuarez at igalia.com>
wrote:

> On Fri, 2019-03-08 at 13:29 -0600, Jason Ekstrand wrote:
>
> 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.
>
>
> I agree, but I understand in this case the second branch is not diverging,
> but converging to the merge case already defined in the selection merge.
>
> I found a couple of similar questions in public github, which were
> resolved by stating that not all OpBranchConditional must be immediately
> preceded by an OpSelectionMerge or OpLoopMerge.
>
> https://github.com/KhronosGroup/glslang/issues/150#issuecomment-178185325
>
> https://github.com/KhronosGroup/SPIRV-Tools/issues/1185#issuecomment-356457613
>

Grumble grumble.... This is a backwards-incompatible SPIR-V change....

Ok, I'll look at this some more in the fresh light.  I'm not sure if the
short-circuit you're doing is correct but we'll see.



> J.A.
>
> --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/20190314/c6232a47/attachment-0001.html>