[Mesa-dev] [PATCH v2] nir: Handle variables dependent on the local work group size.

Jason Ekstrand jason at jlekstrand.net
Mon Nov 12 03:07:24 UTC 2018


On Sun, Nov 11, 2018 at 8:46 PM Karol Herbst <kherbst at redhat.com> wrote:

>  an)
> On Mon, Nov 12, 2018 at 12:37 AM Jason Ekstrand <jason at jlekstrand.net>
> wrote:
> >
> > On November 11, 2018 16:36:16 Karol Herbst <kherbst at redhat.com> wrote:
> >
> > > On Sun, Nov 11, 2018 at 10:48 PM Jason Ekstrand <jason at jlekstrand.net>
> wrote:
> > >>
> > >> On Sun, Nov 11, 2018 at 3:35 PM Plamena Manolova
> > >> <plamena.n.manolova at gmail.com> wrote:
> > >>>
> > >>>
> > >>> Lowering shader variables which depend on the local work group
> > >>> size being available in nir_lower_system_values is only possible
> > >>> if the local work group size isn't variable, otherwise this should
> > >>> be handled by the native driver (if it supports
> > >>> ARB_compute_variable_group_size).
> > >>>
> > >>>
> > >>> Signed-off-by: Plamena Manolova <plamena.n.manolova at gmail.com>
> > >>> ---
> > >>> src/compiler/nir/nir_lower_system_values.c | 22
> ++++++++++++++++++++++
> > >>> 1 file changed, 22 insertions(+)
> > >>>
> > >>>
> > >>> diff --git a/src/compiler/nir/nir_lower_system_values.c
> > >>> b/src/compiler/nir/nir_lower_system_values.c
> > >>> index bde7eb1180..6fdf9f9c51 100644
> > >>> --- a/src/compiler/nir/nir_lower_system_values.c
> > >>> +++ b/src/compiler/nir/nir_lower_system_values.c
> > >>> @@ -77,6 +77,15 @@ convert_block(nir_block *block, nir_builder *b)
> > >>>    *    "The value of gl_GlobalInvocationID is equal to
> > >>>    *    gl_WorkGroupID * gl_WorkGroupSize + gl_LocalInvocationID"
> > >>>    */
> > >>> +
> > >>> +         /*
> > >>> +          * If the local work group size is variable we can't lower
> the global
> > >>> +          * invocation id here.
> > >>
> > >>
> > >> Why not?
> > >>
> > >>>
> > >>>
> > >>> +          */
> > >>> +         if (b->shader->info.cs.local_size_variable) {
> > >>
> > >>
> > >> I didn't realize we'd added a bit for this.  At one point in time,
> Karol
> > >> had patches which had it keyed off of the size being zero.  Having a
> > >> separate bit is probably fine, it just surpised me.
> > >
> > >
> > > yeah.. I guess I choose that, because I had nothing better. But I
> > > guess having the size being 0 is good enough as long as we sure it is
> > > 0 in relevant cases.
> >
> > It's fine. I just thought we'd chosen a different convention bit there's
> > nothing wrong with it. I was just surprised. That's all.
> >
> > >
> > >>>
> > >>>
> > >>> +            break;
> > >>> +         }
> > >>> +
> > >>>   nir_ssa_def *group_size = build_local_group_size(b);
> > >>>   nir_ssa_def *group_id = nir_load_work_group_id(b);
> > >>>   nir_ssa_def *local_id = nir_load_local_invocation_id(b);
> > >>> @@ -115,6 +124,11 @@ convert_block(nir_block *block, nir_builder *b)
> > >>> }
> > >>>
> > >>>
> > >>> case SYSTEM_VALUE_LOCAL_GROUP_SIZE: {
> > >>> +         /* If the local work group size is variable we can't lower
> it here */
> > >>> +         if (b->shader->info.cs.local_size_variable) {
> > >>> +            break;
> > >>> +         }
> > >>> +
> > >>>   sysval = build_local_group_size(b);
> > >>>   break;
> > >>> }
> > >>> @@ -189,6 +203,14 @@ convert_block(nir_block *block, nir_builder *b)
> > >>>   break;
> > >>>
> > >>>
> > >>> case SYSTEM_VALUE_GLOBAL_GROUP_SIZE: {
> > >>> +         /*
> > >>> +          * If the local work group size is variable we can't lower
> the global
> > >>> +          * group size here.
> > >>> +          */
> > >>> +         if (b->shader->info.cs.local_size_variable) {
> > >>> +            break;
> > >>> +         }
> > >>
> > >>
> > >> Why can't we lower the global size?  It seems like we would want the
> below
> > >> multiplication regardless of whether the local size is constant.
> > >
> > > well I am not sure about ARB_compute_variable_group_size, but at least
> > > in CL you know nothing about it at compile time as you specify
> > > everything when you enqueue the kernel. Could be that the number of
> > > work_groups is fixed with ARB_compute_variable_group_size though
> >
> > Why can't you multiply two non-constant things together? Maybe the driver
> > can do something now efficient but it's not clear to me that this isn't
> > what we want.
> >
>
> oh, you meant it that way. Mhh, I actually wrote the patch adding
> build_local_group_size to handle that inside that function (lower to
> constants if known, intrinsics otherwise) as we need the local_size
> for three different system values. I have a patch for that for CL, but
> the patch itself is a bit hacky right now (as in, it doesn't handle it
> inside build_local_group_size).
>
> And I think that patch misses handling for
> SYSTEM_VALUE_GLOBAL_GROUP_SIZE anyhow.
>
> For CL we even want to add a global_invocation_id_offset system value
> and add it when handling SYSTEM_VALUE_GLOBAL_INVOCATION_ID, but that
> we can ignore for now.
>
> Anyhow, I think it would make sense to move the check into
> build_local_group_sized and either return a constant value or an
> intrinsic loading the value from the driver. I just remember vaguely
> that I think somebody brought that up months ago and there was a good
> reason not to do that for every driver?
>

That's what I was thinking.  We don't want to lower load_local_group_size
but then we want build_local_group_size to either build a constant or a
load_local_group_size intrinsic as you said above.

--Jason


> > >
> > >>>
> > >>>
> > >>> +
> > >>>   nir_ssa_def *group_size = build_local_group_size(b);
> > >>>   nir_ssa_def *num_work_groups = nir_load_num_work_groups(b);
> > >>>   sysval = nir_imul(b, group_size, num_work_groups);
> > >>> --
> > >>> 2.11.0
> > >>>
> > >>>
> > >>> _______________________________________________
> > >>> mesa-dev mailing list
> > >>> mesa-dev at lists.freedesktop.org
> > >>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> >
> >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20181111/614c623e/attachment-0001.html>


More information about the mesa-dev mailing list