[Mesa-dev] [PATCH 1/3] nir/opt_intrinsics: Fix values for gl_SubGroupG{e, t}MaskARB

[Neil Roberts]

Thanks for the review. I’ve pushed this first patch to master and I’ll
drop the rest. I’ve also pushed the piglit patch.

Regards,
- Neil

Jason Ekstrand <jason at jlekstrand.net> writes:

> On Tue, Oct 31, 2017 at 10:55 AM, Neil Roberts <nroberts at igalia.com> wrote:
>
>> Previously the values were calculated by just shifting ~0 by the
>> invocation ID. This would end up including bits that are higher than
>> gl_SubGroupSizeARB. The corresponding CTS test effectively requires that
>> these high bits be zero so it was failing. There is a Piglit test as
>> well but this appears to checking the wrong values so it passes.
>>
>> For the two greater-than bitmasks, this patch adds an extra mask with
>> (~0>>(64-gl_SubGroupSizeARB)) to force these bits to zero.
>>
>
> 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.
>
>
>> Fixes: KHR-GL45.shader_ballot_tests.ShaderBallotBitmasks
>>
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=102680#c3
>> Signed-off-by: Neil Roberts <nroberts at igalia.com>
>> ---
>>  src/compiler/nir/nir_opt_intrinsics.c | 24 ++++++++++++++++++++++--
>>  1 file changed, 22 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/compiler/nir/nir_opt_intrinsics.c
>> b/src/compiler/nir/nir_opt_intrinsics.c
>> index 26a0f96..d5fdc51 100644
>> --- a/src/compiler/nir/nir_opt_intrinsics.c
>> +++ b/src/compiler/nir/nir_opt_intrinsics.c
>> @@ -28,6 +28,26 @@
>>   * \file nir_opt_intrinsics.c
>>   */
>>
>> +static nir_ssa_def *
>> +high_subgroup_mask(nir_builder *b,
>> +                   nir_ssa_def *count,
>> +                   uint64_t base_mask)
>> +{
>> +   /* group_mask could probably be calculated more efficiently but we
>> want to
>> +    * be sure not to shift by 64 if the subgroup size is 64 because the
>> GLSL
>> +    * shift operator is undefined in that case.
>
>
> Yeah, I'm pretty sure our hardware will just not shift in that case.
>
>
>> In any case if we were worried
>> +    * about efficency this should probably be done further down because
>> the
>> +    * subgroup size is likely to be known at compile time.
>>
>
> I wouldn't be worried about efficiency.  As I said in an earlier patch,
> this becomes a constant with my series.
>
> Reviewed-by: Jason Ekstrand <jason at jlekstrand.net>
> Cc: mesa-stable at lists.freedesktop.org
>
> Please push soon or maybe I'll just push it myself.
>
>
>> +    */
>> +   nir_ssa_def *subgroup_size = nir_load_subgroup_size(b);
>> +   nir_ssa_def *all_bits = nir_imm_int64(b, ~0ull);
>> +   nir_ssa_def *shift = nir_isub(b, nir_imm_int(b, 64), subgroup_size);
>> +   nir_ssa_def *group_mask = nir_ushr(b, all_bits, shift);
>> +   nir_ssa_def *higher_bits = nir_ishl(b, nir_imm_int64(b, base_mask),
>> count);
>> +
>> +   return nir_iand(b, higher_bits, group_mask);
>> +}
>> +
>>  static bool
>>  opt_intrinsics_impl(nir_function_impl *impl)
>>  {
>> @@ -95,10 +115,10 @@ opt_intrinsics_impl(nir_function_impl *impl)
>>                 replacement = nir_ishl(&b, nir_imm_int64(&b, 1ull), count);
>>                 break;
>>              case nir_intrinsic_load_subgroup_ge_mask:
>> -               replacement = nir_ishl(&b, nir_imm_int64(&b, ~0ull),
>> count);
>> +               replacement = high_subgroup_mask(&b, count, ~0ull);
>>                 break;
>>              case nir_intrinsic_load_subgroup_gt_mask:
>> -               replacement = nir_ishl(&b, nir_imm_int64(&b, ~1ull),
>> count);
>> +               replacement = high_subgroup_mask(&b, count, ~1ull);
>>                 break;
>>              case nir_intrinsic_load_subgroup_le_mask:
>>                 replacement = nir_inot(&b, nir_ishl(&b, nir_imm_int64(&b,
>> ~1ull), count));
>> --
>> 2.9.5
>>
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 818 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20171031/bfbbae61/attachment.sig>