<div dir="ltr"><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Apr 18, 2019 at 9:13 AM Juan A. Suarez Romero <<a href="mailto:jasuarez@igalia.com">jasuarez@igalia.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div style="text-align:left;direction:ltr"><div>On Thu, 2019-03-14 at 11:25 -0500, Jason Ekstrand wrote:</div><blockquote type="cite" style="margin:0px 0px 0px 0.8ex;border-left:2px solid rgb(114,159,207);padding-left:1ex"><div dir="ltr"><div>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.</div><div><br></div><div><br></div></div></blockquote><div><br></div><div>Did you have time to check this?</div></div></blockquote><div><br></div><div>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.....</div><div><br></div><div>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.<br></div><div><br></div><div>--Jason<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div style="text-align:left;direction:ltr"><div><br></div><div> J.A.</div><div><br></div><blockquote type="cite" style="margin:0px 0px 0px 0.8ex;border-left:2px solid rgb(114,159,207);padding-left:1ex"><div dir="ltr"><div>--Jason<br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Mar 6, 2019 at 5:25 AM Juan A. Suarez Romero <<a href="mailto:jasuarez@igalia.com" target="_blank">jasuarez@igalia.com</a>> wrote:<br></div><blockquote type="cite" style="margin:0px 0px 0px 0.8ex;border-left:2px solid rgb(114,159,207);padding-left:1ex">This fixes the case when the SPIR-V code has two nested conditional<br>
branches, but only one selection merge:<br>
<br>
[...]<br>
%1 = OpLabel<br>
OpSelectionMerge %2 None<br>
OpBranchConditional %3 %4 %2<br>
%4 = OpLabel<br>
OpBranchConditional %3 %5 %2<br>
%5 = OpLabel<br>
OpBranch %2<br>
%2 = OpLabel<br>
[...]<br>
<br>
In the second OpBranchConditional, as the else-part is the end<br>
block (started in the first OpBranchConditional) we can just follow the<br>
then-part.<br>
<br>
This fixes dEQP-VK.vkrunner.controlflow.2-obc-triangle-triangle<br>
<br>
CC: Jason Ekstrand <<a href="mailto:jason@jlekstrand.net" target="_blank">jason@jlekstrand.net</a>><br>
---<br>
src/compiler/spirv/vtn_cfg.c | 11 ++++++++++-<br>
1 file changed, 10 insertions(+), 1 deletion(-)<br>
<br>
diff --git a/src/compiler/spirv/vtn_cfg.c b/src/compiler/spirv/vtn_cfg.c<br>
index 7868eeb60bc..f749118efbe 100644<br>
--- a/src/compiler/spirv/vtn_cfg.c<br>
+++ b/src/compiler/spirv/vtn_cfg.c<br>
@@ -605,7 +605,16 @@ vtn_cfg_walk_blocks(struct vtn_builder *b, struct list_head *cf_list,<br>
}<br>
} else if (if_stmt->then_type == vtn_branch_type_none &&<br>
if_stmt->else_type == vtn_branch_type_none) {<br>
- /* Neither side of the if is something we can short-circuit. */<br>
+ /* Neither side of the if is something we can short-circuit,<br>
+ * unless one of the blocks is the end block. */<br>
+ if (then_block == end) {<br>
+ block = else_block;<br>
+ continue;<br>
+ } else if (else_block == end) {<br>
+ block = then_block;<br>
+ continue;<br>
+ }<br>
+<br>
vtn_assert((*block->merge & SpvOpCodeMask) == SpvOpSelectionMerge);<br>
struct vtn_block *merge_block =<br>
vtn_value(b, block->merge[1], vtn_value_type_block)->block;<br>
</blockquote></div></blockquote></div>
</blockquote></div></div>