<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN" "http://www.w3.org/TR/html4/loose.dtd"> <html> <body> <div dir="auto"> <div dir="auto"><span style="font-family: sans-serif; font-size: 10pt;">On November 12, 2018 08:25:50 "Manolova, Plamena" <plamena.manolova@intel.com> wrote:</span></div><div id="aqm-original" style="color: black;"><div class="aqm-original-body"><div style="color: black;"> <blockquote type="cite" class="gmail_quote" style="margin: 0 0 0 0.75ex; border-left: 1px solid #808080; padding-left: 0.75ex;"> <div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr">Hi Jason,<div>Thank you so much for reviewing! In my initial series for ARB_compute_variable_group_size</div><div>(<a href="https://patchwork.freedesktop.org/patch/228130">https://patchwork.freedesktop.org/patch/228130</a>) from which this is extracted, I moved</div><div>lowering these variables to brw_nir_lower_cs_intrinsics and did what you're suggesting i.e.<br></div><div>I used the load_local_group_size intrinsic. My reasoning was that this might give other drivers</div><div>some flexibility on how they can handle this, but if it makes more sense to keep this in</div><div>nir_lower_system_values I'm happy to rework my patch.</div></div></div></div></div></div></div></div></div></div></div></div></div></blockquote></div></div></div><div dir="auto"><br></div><div dir="auto">Yeah, let's keep it general unless we have a reason to make it specific.</div><div dir="auto"><br></div><div dir="auto">--Jason</div><div dir="auto"><br></div><div id="aqm-original" style="color: black;" dir="auto"><div class="aqm-original-body"><div style="color: black;"><blockquote type="cite" class="gmail_quote" style="margin: 0 0 0 0.75ex; border-left: 1px solid #808080; padding-left: 0.75ex;"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div></div><div>Thank you,</div><div>Pam</div></div></div></div></div></div></div></div></div></div></div></div></div><br><div class="gmail_quote"><div dir="ltr">On Mon, Nov 12, 2018 at 5:07 AM Jason Ekstrand <<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Sun, Nov 11, 2018 at 8:46 PM Karol Herbst <<a href="mailto:kherbst@redhat.com" target="_blank">kherbst@redhat.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> an)<br> On Mon, Nov 12, 2018 at 12:37 AM Jason Ekstrand <<a href="mailto:jason@jlekstrand.net" target="_blank">jason@jlekstrand.net</a>> wrote:<br> ><br> > On November 11, 2018 16:36:16 Karol Herbst <<a href="mailto:kherbst@redhat.com" target="_blank">kherbst@redhat.com</a>> wrote:<br> ><br> > > On Sun, Nov 11, 2018 at 10:48 PM Jason Ekstrand <<a href="mailto:jason@jlekstrand.net" target="_blank">jason@jlekstrand.net</a>> wrote:<br> > >><br> > >> On Sun, Nov 11, 2018 at 3:35 PM Plamena Manolova<br> > >> <<a href="mailto:plamena.n.manolova@gmail.com" target="_blank">plamena.n.manolova@gmail.com</a>> wrote:<br> > >>><br> > >>><br> > >>> Lowering shader variables which depend on the local work group<br> > >>> size being available in nir_lower_system_values is only possible<br> > >>> if the local work group size isn't variable, otherwise this should<br> > >>> be handled by the native driver (if it supports<br> > >>> ARB_compute_variable_group_size).<br> > >>><br> > >>><br> > >>> Signed-off-by: Plamena Manolova <<a href="mailto:plamena.n.manolova@gmail.com" target="_blank">plamena.n.manolova@gmail.com</a>><br> > >>> ---<br> > >>> src/compiler/nir/nir_lower_system_values.c | 22 ++++++++++++++++++++++<br> > >>> 1 file changed, 22 insertions(+)<br> > >>><br> > >>><br> > >>> diff --git a/src/compiler/nir/nir_lower_system_values.c<br> > >>> b/src/compiler/nir/nir_lower_system_values.c<br> > >>> index bde7eb1180..6fdf9f9c51 100644<br> > >>> --- a/src/compiler/nir/nir_lower_system_values.c<br> > >>> +++ b/src/compiler/nir/nir_lower_system_values.c<br> > >>> @@ -77,6 +77,15 @@ convert_block(nir_block *block, nir_builder *b)<br> > >>> * "The value of gl_GlobalInvocationID is equal to<br> > >>> * gl_WorkGroupID * gl_WorkGroupSize + gl_LocalInvocationID"<br> > >>> */<br> > >>> +<br> > >>> + /*<br> > >>> + * If the local work group size is variable we can't lower the global<br> > >>> + * invocation id here.<br> > >><br> > >><br> > >> Why not?<br> > >><br> > >>><br> > >>><br> > >>> + */<br> > >>> + if (b->shader->info.cs.local_size_variable) {<br> > >><br> > >><br> > >> I didn't realize we'd added a bit for this. At one point in time, Karol<br> > >> had patches which had it keyed off of the size being zero. Having a<br> > >> separate bit is probably fine, it just surpised me.<br> > ><br> > ><br> > > yeah.. I guess I choose that, because I had nothing better. But I<br> > > guess having the size being 0 is good enough as long as we sure it is<br> > > 0 in relevant cases.<br> ><br> > It's fine. I just thought we'd chosen a different convention bit there's<br> > nothing wrong with it. I was just surprised. That's all.<br> ><br> > ><br> > >>><br> > >>><br> > >>> + break;<br> > >>> + }<br> > >>> +<br> > >>> nir_ssa_def *group_size = build_local_group_size(b);<br> > >>> nir_ssa_def *group_id = nir_load_work_group_id(b);<br> > >>> nir_ssa_def *local_id = nir_load_local_invocation_id(b);<br> > >>> @@ -115,6 +124,11 @@ convert_block(nir_block *block, nir_builder *b)<br> > >>> }<br> > >>><br> > >>><br> > >>> case SYSTEM_VALUE_LOCAL_GROUP_SIZE: {<br> > >>> + /* If the local work group size is variable we can't lower it here */<br> > >>> + if (b->shader->info.cs.local_size_variable) {<br> > >>> + break;<br> > >>> + }<br> > >>> +<br> > >>> sysval = build_local_group_size(b);<br> > >>> break;<br> > >>> }<br> > >>> @@ -189,6 +203,14 @@ convert_block(nir_block *block, nir_builder *b)<br> > >>> break;<br> > >>><br> > >>><br> > >>> case SYSTEM_VALUE_GLOBAL_GROUP_SIZE: {<br> > >>> + /*<br> > >>> + * If the local work group size is variable we can't lower the global<br> > >>> + * group size here.<br> > >>> + */<br> > >>> + if (b->shader->info.cs.local_size_variable) {<br> > >>> + break;<br> > >>> + }<br> > >><br> > >><br> > >> Why can't we lower the global size? It seems like we would want the below<br> > >> multiplication regardless of whether the local size is constant.<br> > ><br> > > well I am not sure about ARB_compute_variable_group_size, but at least<br> > > in CL you know nothing about it at compile time as you specify<br> > > everything when you enqueue the kernel. Could be that the number of<br> > > work_groups is fixed with ARB_compute_variable_group_size though<br> ><br> > Why can't you multiply two non-constant things together? Maybe the driver<br> > can do something now efficient but it's not clear to me that this isn't<br> > what we want.<br> ><br> <br> oh, you meant it that way. Mhh, I actually wrote the patch adding<br> build_local_group_size to handle that inside that function (lower to<br> constants if known, intrinsics otherwise) as we need the local_size<br> for three different system values. I have a patch for that for CL, but<br> the patch itself is a bit hacky right now (as in, it doesn't handle it<br> inside build_local_group_size).<br> <br> And I think that patch misses handling for<br> SYSTEM_VALUE_GLOBAL_GROUP_SIZE anyhow.<br> <br> For CL we even want to add a global_invocation_id_offset system value<br> and add it when handling SYSTEM_VALUE_GLOBAL_INVOCATION_ID, but that<br> we can ignore for now.<br> <br> Anyhow, I think it would make sense to move the check into<br> build_local_group_sized and either return a constant value or an<br> intrinsic loading the value from the driver. I just remember vaguely<br> that I think somebody brought that up months ago and there was a good<br> reason not to do that for every driver?<br></blockquote><div><br></div><div>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.</div><div><br></div><div>--Jason<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> > ><br> > >>><br> > >>><br> > >>> +<br> > >>> nir_ssa_def *group_size = build_local_group_size(b);<br> > >>> nir_ssa_def *num_work_groups = nir_load_num_work_groups(b);<br> > >>> sysval = nir_imul(b, group_size, num_work_groups);<br> > >>> --<br> > >>> 2.11.0<br> > >>><br> > >>><br> > >>> _______________________________________________<br> > >>> mesa-dev mailing list<br> > >>> <a href="mailto:mesa-dev@lists.freedesktop.org" target="_blank">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/mailman/listinfo/mesa-dev</a><br> ><br> ><br> ><br> </blockquote></div></div> _______________________________________________<br> mesa-dev mailing list<br> <a href="mailto:mesa-dev@lists.freedesktop.org" target="_blank">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/mailman/listinfo/mesa-dev</a><br> </blockquote></div> </blockquote> </div> </div>  </div><div dir="auto"><br></div> </div></body> </html>