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

[Juan A. Suarez Romero]

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 before the control flow diverges and a merge block 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-178185325https://github.com/KhronosGroup/SPIRV-Tools/issues/1185#issuecomment-356457613
	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/e6010723/attachment.html>