<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Tue, Oct 31, 2017 at 10:55 AM, Neil Roberts <span dir="ltr"><<a href="mailto:nroberts@igalia.com" target="_blank">nroberts@igalia.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Previously the values were calculated by just shifting ~0 by the<br>
invocation ID. This would end up including bits that are higher than<br>
gl_SubGroupSizeARB. The corresponding CTS test effectively requires that<br>
these high bits be zero so it was failing. There is a Piglit test as<br>
well but this appears to checking the wrong values so it passes.<br>
<br>
For the two greater-than bitmasks, this patch adds an extra mask with<br>
(~0>>(64-gl_SubGroupSizeARB)) to force these bits to zero.<br></blockquote><div><br></div><div>We had a big discussion about this in the office yesterday. :-) From my reading of the GL_ARB_shader_ballot and GL_NV_shader_thread_group specs, it looked like the current behavior was correct. However, I'm very glad to see that I was wrong because it means that Vulkan and GL are consistent with each other. :) It'll be a bit painful to rebase my subgroup work on top of this but I think I'd prefer it if we land this first as it will actually make some things a bit simpler even though it will conflict madly.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Fixes: KHR-GL45.shader_ballot_tests.<wbr>ShaderBallotBitmasks<br>
<br>
Bugzilla: <a href="https://bugs.freedesktop.org/show_bug.cgi?id=102680#c3" rel="noreferrer" target="_blank">https://bugs.freedesktop.org/<wbr>show_bug.cgi?id=102680#c3</a><br>
Signed-off-by: Neil Roberts <<a href="mailto:nroberts@igalia.com">nroberts@igalia.com</a>><br>
---<br>
src/compiler/nir/nir_opt_<wbr>intrinsics.c | 24 ++++++++++++++++++++++--<br>
1 file changed, 22 insertions(+), 2 deletions(-)<br>
<br>
diff --git a/src/compiler/nir/nir_opt_<wbr>intrinsics.c b/src/compiler/nir/nir_opt_<wbr>intrinsics.c<br>
index 26a0f96..d5fdc51 100644<br>
--- a/src/compiler/nir/nir_opt_<wbr>intrinsics.c<br>
+++ b/src/compiler/nir/nir_opt_<wbr>intrinsics.c<br>
@@ -28,6 +28,26 @@<br>
* \file nir_opt_intrinsics.c<br>
*/<br>
<br>
+static nir_ssa_def *<br>
+high_subgroup_mask(nir_<wbr>builder *b,<br>
+ nir_ssa_def *count,<br>
+ uint64_t base_mask)<br>
+{<br>
+ /* group_mask could probably be calculated more efficiently but we want to<br>
+ * be sure not to shift by 64 if the subgroup size is 64 because the GLSL<br>
+ * shift operator is undefined in that case.</blockquote><div><br></div><div>Yeah, I'm pretty sure our hardware will just not shift in that case.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> In any case if we were worried<br>
+ * about efficency this should probably be done further down because the<br>
+ * subgroup size is likely to be known at compile time.<br></blockquote><div><br></div><div>I wouldn't be worried about efficiency. As I said in an earlier patch, this becomes a constant with my series.</div><div><br></div><div>Reviewed-by: Jason Ekstrand <<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>></div><div>Cc: <a href="mailto:mesa-stable@lists.freedesktop.org">mesa-stable@lists.freedesktop.org</a></div><div><br></div><div>Please push soon or maybe I'll just push it myself.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ */<br>
+ nir_ssa_def *subgroup_size = nir_load_subgroup_size(b);<br>
+ nir_ssa_def *all_bits = nir_imm_int64(b, ~0ull);<br>
+ nir_ssa_def *shift = nir_isub(b, nir_imm_int(b, 64), subgroup_size);<br>
+ nir_ssa_def *group_mask = nir_ushr(b, all_bits, shift);<br>
+ nir_ssa_def *higher_bits = nir_ishl(b, nir_imm_int64(b, base_mask), count);<br>
+<br>
+ return nir_iand(b, higher_bits, group_mask);<br>
+}<br>
+<br>
static bool<br>
opt_intrinsics_impl(nir_<wbr>function_impl *impl)<br>
{<br>
@@ -95,10 +115,10 @@ opt_intrinsics_impl(nir_<wbr>function_impl *impl)<br>
replacement = nir_ishl(&b, nir_imm_int64(&b, 1ull), count);<br>
break;<br>
case nir_intrinsic_load_subgroup_<wbr>ge_mask:<br>
- replacement = nir_ishl(&b, nir_imm_int64(&b, ~0ull), count);<br>
+ replacement = high_subgroup_mask(&b, count, ~0ull);<br>
break;<br>
case nir_intrinsic_load_subgroup_<wbr>gt_mask:<br>
- replacement = nir_ishl(&b, nir_imm_int64(&b, ~1ull), count);<br>
+ replacement = high_subgroup_mask(&b, count, ~1ull);<br>
break;<br>
case nir_intrinsic_load_subgroup_<wbr>le_mask:<br>
replacement = nir_inot(&b, nir_ishl(&b, nir_imm_int64(&b, ~1ull), count));<br>
<span class="HOEnZb"><font color="#888888">--<br>
2.9.5<br>
<br>
______________________________<wbr>_________________<br>
mesa-dev mailing list<br>
<a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a><br>
<a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/<wbr>mailman/listinfo/mesa-dev</a><br>
</font></span></blockquote></div><br></div></div>