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

[Jason Ekstrand]

On Thu, Apr 18, 2019 at 9:13 AM Juan A. Suarez Romero 
wrote:

> On Thu, 2019-03-14 at 11:25 -0500, Jason Ekstrand wrote:
>
> Looking at this a bit more, I'm not sure that just short-circuiting
> actually covers all the cases.  Unfortunately, we don't know what all the
> cases are because the SPIR-V spec doesn't say.  I'm trying to work towards
> fixing that right now.
>
>
>
> Did you have time to check this?
>

I had a branch where I was working on a full solution but then I ran into
"what does the spec even mean?" issues, got the SPIR-V working group to
chatter a bit, and then haven't heard anything in weeks.  I think the best
thing for us to do would be to assume "worst case" and try to handle as
arbitrary CFGs as possible.  I don't think just an early return will do
that though.  Looking at my git repo, I'm not finding a branch.

The approach I took (if I can remember correctly) was to add a new
vtn_branch_type_merge which was used for the merge node of the current
construct.  (Or maybe I only used it for if-merges, I don't remember.)
Then we would try to make vtn_cfg handle an arbitrary amount of divergence
so long as everything eventually either jumps to a special target (such as
a break/continue) or goes through the merge.  That may end up being roughly
equivalent to what you've tried to do in this patch but I think it's more
robust to handle as it's own branch type than to just have a short-circuit.

--Jason


>
> J.A.
>
> --Jason
>
> On Wed, Mar 6, 2019 at 5:25 AM Juan A. Suarez Romero 
> 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 
> ---
>  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;
>
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Juan A. Suarez Romero]

On Thu, 2019-03-14 at 11:25 -0500, Jason Ekstrand wrote:
> Looking at this a bit more, I'm not sure that just short-circuiting actually 
> covers all the cases.  Unfortunately, we don't know what all the cases are 
> because the SPIR-V spec doesn't say.  I'm trying to work towards fixing that 
> right now.
> 
> 

Did you have time to check this?
J.A.
> --Jason
> 
> 
> On Wed, Mar 6, 2019 at 5:25 AM Juan A. Suarez Romero  
> 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 
> > 
> > ---
> > 
> >  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;
> > 
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Jason Ekstrand]

Looking at this a bit more, I'm not sure that just short-circuiting
actually covers all the cases.  Unfortunately, we don't know what all the
cases are because the SPIR-V spec doesn't say.  I'm trying to work towards
fixing that right now.

--Jason

On Wed, Mar 6, 2019 at 5:25 AM Juan A. Suarez Romero 
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 
> ---
>  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

Re: [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 
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 
> 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-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" 
> 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 
> > > ---
> > > 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;
> > > +}
> > > +
> 

Re: [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  
> 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"  
> > > 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 
> > 
> > > > ---
> > 
> > > > 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) {
> > 
> > 

Re: [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 
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.

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

Re: [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"  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 
> > ---
> > 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

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

[Jason Ekstrand]

Woah, is this legal SPIR-V? I think a second OpSelectionMerge is required.

--Jason


On March 6, 2019 05:25:26 "Juan A. Suarez Romero"  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 
---
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

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

[Juan A. Suarez Romero]

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