Hi Jason, Thank you so much for reviewing! In my initial series for ARB_compute_variable_group_size (https://patchwork.freedesktop.org/patch/228130) from which this is extracted, I moved lowering these variables to brw_nir_lower_cs_intrinsics and did what you're suggesting i.e. I used the load_local_group_size intrinsic. My reasoning was that this might give other drivers some flexibility on how they can handle this, but if it makes more sense to keep this in nir_lower_system_values I'm happy to rework my patch.
Thank you, Pam On Mon, Nov 12, 2018 at 5:07 AM Jason Ekstrand <[email protected]> wrote: > On Sun, Nov 11, 2018 at 8:46 PM Karol Herbst <[email protected]> wrote: > >> an) >> On Mon, Nov 12, 2018 at 12:37 AM Jason Ekstrand <[email protected]> >> wrote: >> > >> > On November 11, 2018 16:36:16 Karol Herbst <[email protected]> wrote: >> > >> > > On Sun, Nov 11, 2018 at 10:48 PM Jason Ekstrand <[email protected]> >> wrote: >> > >> >> > >> On Sun, Nov 11, 2018 at 3:35 PM Plamena Manolova >> > >> <[email protected]> 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 <[email protected]> >> > >>> --- >> > >>> 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 >> > >>> [email protected] >> > >>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev >> > >> > >> > >> > _______________________________________________ > mesa-dev mailing list > [email protected] > https://lists.freedesktop.org/mailman/listinfo/mesa-dev >
_______________________________________________ mesa-dev mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/mesa-dev
