<div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Fri, Jan 11, 2019 at 10:19 AM Kenneth Graunke <<a href="mailto:kenneth@whitecape.org">kenneth@whitecape.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Wednesday, January 9, 2019 5:33:22 PM PST Ian Romanick wrote:<br>
> On 1/8/19 9:57 PM, Kenneth Graunke wrote:<br>
> > On Tuesday, December 4, 2018 10:26:43 AM PST Karol Herbst wrote:<br>
> >> the naming is a bit confusing no matter how you look at it. Within SPIR-V<br>
> >> "global" memory is memory accessible from all threads. glsl "global" memory<br>
> >> normally refers to shader thread private memory declared at global scope. As<br>
> >> we already use "shared" for memory shared across all thrads of a work group<br>
> >> the solution where everybody could be happy with is to rename "global" to<br>
> >> "private" and use "global" later for memory usually stored within system<br>
> >> accessible memory (be it VRAM or system RAM if keeping SVM in mind).<br>
> >> glsl "local" memory is memory only accessible within a function, while SPIR-V<br>
> >> "local" memory is memory accessible within the same workgroup.<br>
> >><br>
> >> v2: rename local to function as well<br>
> >><br>
> >> Signed-off-by: Karol Herbst <<a href="mailto:kherbst@redhat.com" target="_blank">kherbst@redhat.com</a>><br>
> > <br>
> > I strongly dislike this patch, and I think we ought to revert it.<br>
> > <br>
> > This probably makes sense from an OpenCL memory-model view of the world,<br>
> > but it's really confusing from a compiler or general programming point<br>
> > of view.<br>
> > <br>
> > /Everybody/ knows what a local variable is.  It's one of the most used<br>
> > concepts in programming.  Calling it nir_var_function is very confusing.<br>
> > The variable is a...function?  Maybe it's a function pointer?  Neither<br>
> > of those things even exist in GLSL, so...what the heck is it?<br>
> > <br>
> > Renaming global scope variables to "private" is also confusing IMO.<br>
> > They're certainly not private to a function.  They're globally<br>
> > accessible by anything in the whole shader.  I'll admit "global" isn't<br>
> > a great name either.<br>
> <br>
> It seems like the concepts we're after a function local and thread<br>
> local, so why not nir_var_thread_local (for old nir_var_global) and<br>
> nir_var_function_local (for old nir_var_local).  When "global" is<br>
> reintroduced to mean thread global, we could add it as<br>
> nir_var_thread_global.  That seems to match at least one reasonable view<br>
> of a storage hierarchy.<br>
<br>
Those names (nir_var_func_local, nir_var_thread_local, and<br>
nir_var_thread_global) make more sense to me than private/function.<br>
<br>
Another option is `nir_var_local_temp` and `nir_var_shader_temp`,<br>
indicating that they're just temporary variables, and not anything<br>
with special semantics like memory.  shader_temp would pair well with<br>
the existing shader_in/shader_out, since they have the same scope.<br>
<br>
I might also consider adding 'mem' to variables representing memory.<br>
<br>
So that would look like...<br>
<br>
   nir_var_shader_in<br>
   nir_var_shader_out<br>
   nir_var_shader_temp  (formerly local/function)<br>
   nir_var_local_temp   (formerly global/private)<br></blockquote><div><br></div><div>Are those flipped?<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
   nir_var_uniform<br>
   nir_var_system_value<br>
   nir_var_mem_ubo      (added mem)<br>
   nir_var_mem_ssbo     (added mem)<br>
   nir_var_mem_shared   (added mem)<br>
   nir_var_mem_global   (the new global memory type being introduced)<br>
<br>
How does that look?<br></blockquote><div><br></div><div>I think I kind of like having "mem" be on external things.  Shared is a little weird there because it never leaves the chip so is it mem or shader?<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
We may also want to rename the nir->globals list, or<br>
nir_lower_global_vars_to_local and nir_opt_global_to_local.  Not sure.<br></blockquote><div><br></div><div>Yes, whatever we do, we should make those lists more consistent. <br></div></div></div>