On Thu, Mar 14, 2019 at 4:09 AM Juan A. Suarez Romero <jasua...@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 <jasua...@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" <jasua...@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 <ja...@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 > > > > > > > >
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
