Re: [Mesa-dev] [PATCH 05/12] nir: rename global/local to private/function memory
On Fri, Jan 11, 2019 at 8:50 PM Kenneth Graunke wrote: > > On Friday, January 11, 2019 9:32:20 AM PST Eric Anholt wrote: > > Jason Ekstrand writes: > > > > > On Fri, Jan 11, 2019 at 11:11 AM Kenneth Graunke > > > wrote: > > > > > >> On Friday, January 11, 2019 8:33:41 AM PST Jason Ekstrand wrote: > > >> > On Fri, Jan 11, 2019 at 10:19 AM Kenneth Graunke wrote: > > >> > > Those names (nir_var_func_local, nir_var_thread_local, and > > >> > > nir_var_thread_global) make more sense to me than private/function. > > >> > > > > >> > > Another option is `nir_var_local_temp` and `nir_var_shader_temp`, > > >> > > indicating that they're just temporary variables, and not anything > > >> > > with special semantics like memory. shader_temp would pair well with > > >> > > the existing shader_in/shader_out, since they have the same scope. > > >> > > > > >> > > I might also consider adding 'mem' to variables representing memory. > > >> > > > > >> > > So that would look like... > > >> > > > > >> > >nir_var_shader_in > > >> > >nir_var_shader_out > > >> > >nir_var_shader_temp (formerly local/function) > > >> > >nir_var_local_temp (formerly global/private) > > >> > > > > >> > > > >> > Are those flipped? > > >> > > >> Gah! Sorry. Yes. > > >> > > >>nir_var_shader_in > > >>nir_var_shader_out > > >>nir_var_shader_temp (formerly global/private) > > >>nir_var_local_temp (formerly local/function) > > >>nir_var_uniform > > >>nir_var_system_value > > >>nir_var_mem_ubo (added mem) > > >>nir_var_mem_ssbo (added mem) > > >>nir_var_mem_shared (added mem) > > >>nir_var_mem_global (the new global memory type being introduced) > > >> > > > > > > I can work with that. I do think I'd mildly prefer function_temp over > > > local_temp but I think the adding of _temp is an improvement. > > > > I like the _temp suggestion a lot! And I think I'm also mildly in favor > > of function_temp. > > > > (Also, thanks to taking naming seriously, to everyone involved here. > > It's hard.) > > Yep, it's not easy. Karol, sorry for being grumpy the other day, it > wasn't a very constructive email on my part. I agree that the names > should change, for all the reasons you suggested. > no worries. I should have probably pinged a few more people, especially with a patch like that. > > --Ken ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 05/12] nir: rename global/local to private/function memory
On Fri, Jan 11, 2019 at 1:55 PM Kenneth Graunke wrote: > On Friday, January 11, 2019 8:33:41 AM PST Jason Ekstrand wrote: > > 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? > > On Intel GPUs, "shared" maps to a concept called "Shared Local Memory". > So I tend to think of it as memory :) > > It's not perfect, though. While most shader_in/shader_out end up being > thread-local, shader_out in a TCS is actually shared across threads (and > pre-dates the shared keyword). > > I'm sort of inclined to leave that alone for now unless you think we > ought to do something about it. > If by "leave it alone" you mean leave _mem on there, that's fine with me. It was more of a philosophical question than an actual request for any sort of change. > > > We may also want to rename the nir->globals list, or > > > nir_lower_global_vars_to_local and nir_opt_global_to_local. Not sure. > > > > > > > Yes, whatever we do, we should make those lists more consistent. > > > > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 05/12] nir: rename global/local to private/function memory
On Friday, January 11, 2019 8:33:41 AM PST Jason Ekstrand wrote: > 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? On Intel GPUs, "shared" maps to a concept called "Shared Local Memory". So I tend to think of it as memory :) It's not perfect, though. While most shader_in/shader_out end up being thread-local, shader_out in a TCS is actually shared across threads (and pre-dates the shared keyword). I'm sort of inclined to leave that alone for now unless you think we ought to do something about it. > > We may also want to rename the nir->globals list, or > > nir_lower_global_vars_to_local and nir_opt_global_to_local. Not sure. > > > > Yes, whatever we do, we should make those lists more consistent. > signature.asc Description: This is a digitally signed message part. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 05/12] nir: rename global/local to private/function memory
On Fri, Jan 11, 2019 at 1:50 PM Kenneth Graunke wrote: > On Friday, January 11, 2019 9:32:20 AM PST Eric Anholt wrote: > > Jason Ekstrand writes: > > > > > On Fri, Jan 11, 2019 at 11:11 AM Kenneth Graunke < > kenn...@whitecape.org> > > > wrote: > > > > > >> On Friday, January 11, 2019 8:33:41 AM PST Jason Ekstrand wrote: > > >> > On Fri, Jan 11, 2019 at 10:19 AM Kenneth Graunke wrote: > > >> > > Those names (nir_var_func_local, nir_var_thread_local, and > > >> > > nir_var_thread_global) make more sense to me than > private/function. > > >> > > > > >> > > Another option is `nir_var_local_temp` and `nir_var_shader_temp`, > > >> > > indicating that they're just temporary variables, and not anything > > >> > > with special semantics like memory. shader_temp would pair well > with > > >> > > the existing shader_in/shader_out, since they have the same scope. > > >> > > > > >> > > I might also consider adding 'mem' to variables representing > memory. > > >> > > > > >> > > So that would look like... > > >> > > > > >> > >nir_var_shader_in > > >> > >nir_var_shader_out > > >> > >nir_var_shader_temp (formerly local/function) > > >> > >nir_var_local_temp (formerly global/private) > > >> > > > > >> > > > >> > Are those flipped? > > >> > > >> Gah! Sorry. Yes. > > >> > > >>nir_var_shader_in > > >>nir_var_shader_out > > >>nir_var_shader_temp (formerly global/private) > > >>nir_var_local_temp (formerly local/function) > > >>nir_var_uniform > > >>nir_var_system_value > > >>nir_var_mem_ubo (added mem) > > >>nir_var_mem_ssbo (added mem) > > >>nir_var_mem_shared (added mem) > > >>nir_var_mem_global (the new global memory type being introduced) > > >> > > > > > > I can work with that. I do think I'd mildly prefer function_temp over > > > local_temp but I think the adding of _temp is an improvement. > > > > I like the _temp suggestion a lot! And I think I'm also mildly in favor > > of function_temp. > > > > (Also, thanks to taking naming seriously, to everyone involved here. > > It's hard.) > > Yep, it's not easy. Karol, sorry for being grumpy the other day, it > wasn't a very constructive email on my part. I agree that the names > should change, for all the reasons you suggested. > > I'm fine with nir_var_function_temp / nir_var_func_temp if people > prefer that to local_temp. Closer to Karol's original suggestion, > but I think "temp" clarifies it a bit. shader_in is an input in the > shader, function_temp is a temp in a function. > > Sound good? Should I do the sed-job and send patches? > Go for it. Mind also renaming the variable lists? I don't care if it's one patch or two. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 05/12] nir: rename global/local to private/function memory
On Friday, January 11, 2019 9:32:20 AM PST Eric Anholt wrote: > Jason Ekstrand writes: > > > On Fri, Jan 11, 2019 at 11:11 AM Kenneth Graunke > > wrote: > > > >> On Friday, January 11, 2019 8:33:41 AM PST Jason Ekstrand wrote: > >> > On Fri, Jan 11, 2019 at 10:19 AM Kenneth Graunke wrote: > >> > > Those names (nir_var_func_local, nir_var_thread_local, and > >> > > nir_var_thread_global) make more sense to me than private/function. > >> > > > >> > > Another option is `nir_var_local_temp` and `nir_var_shader_temp`, > >> > > indicating that they're just temporary variables, and not anything > >> > > with special semantics like memory. shader_temp would pair well with > >> > > the existing shader_in/shader_out, since they have the same scope. > >> > > > >> > > I might also consider adding 'mem' to variables representing memory. > >> > > > >> > > So that would look like... > >> > > > >> > >nir_var_shader_in > >> > >nir_var_shader_out > >> > >nir_var_shader_temp (formerly local/function) > >> > >nir_var_local_temp (formerly global/private) > >> > > > >> > > >> > Are those flipped? > >> > >> Gah! Sorry. Yes. > >> > >>nir_var_shader_in > >>nir_var_shader_out > >>nir_var_shader_temp (formerly global/private) > >>nir_var_local_temp (formerly local/function) > >>nir_var_uniform > >>nir_var_system_value > >>nir_var_mem_ubo (added mem) > >>nir_var_mem_ssbo (added mem) > >>nir_var_mem_shared (added mem) > >>nir_var_mem_global (the new global memory type being introduced) > >> > > > > I can work with that. I do think I'd mildly prefer function_temp over > > local_temp but I think the adding of _temp is an improvement. > > I like the _temp suggestion a lot! And I think I'm also mildly in favor > of function_temp. > > (Also, thanks to taking naming seriously, to everyone involved here. > It's hard.) Yep, it's not easy. Karol, sorry for being grumpy the other day, it wasn't a very constructive email on my part. I agree that the names should change, for all the reasons you suggested. I'm fine with nir_var_function_temp / nir_var_func_temp if people prefer that to local_temp. Closer to Karol's original suggestion, but I think "temp" clarifies it a bit. shader_in is an input in the shader, function_temp is a temp in a function. Sound good? Should I do the sed-job and send patches? --Ken signature.asc Description: This is a digitally signed message part. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 05/12] nir: rename global/local to private/function memory
On Fri, Jan 11, 2019 at 9:11 AM Kenneth Graunke wrote: > > On Friday, January 11, 2019 8:33:41 AM PST Jason Ekstrand wrote: > > On Fri, Jan 11, 2019 at 10:19 AM Kenneth Graunke wrote: > > > Those names (nir_var_func_local, nir_var_thread_local, and > > > nir_var_thread_global) make more sense to me than private/function. > > > > > > Another option is `nir_var_local_temp` and `nir_var_shader_temp`, > > > indicating that they're just temporary variables, and not anything > > > with special semantics like memory. shader_temp would pair well with > > > the existing shader_in/shader_out, since they have the same scope. > > > > > > I might also consider adding 'mem' to variables representing memory. > > > > > > So that would look like... > > > > > >nir_var_shader_in > > >nir_var_shader_out > > >nir_var_shader_temp (formerly local/function) > > >nir_var_local_temp (formerly global/private) > > > > > > > Are those flipped? > > Gah! Sorry. Yes. > >nir_var_shader_in >nir_var_shader_out >nir_var_shader_temp (formerly global/private) >nir_var_local_temp (formerly local/function) >nir_var_uniform >nir_var_system_value >nir_var_mem_ubo (added mem) >nir_var_mem_ssbo (added mem) >nir_var_mem_shared (added mem) >nir_var_mem_global (the new global memory type being introduced) That sounds good to me. Thanks for coming up with those names. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 05/12] nir: rename global/local to private/function memory
Jason Ekstrand writes: > On Fri, Jan 11, 2019 at 11:11 AM Kenneth Graunke > wrote: > >> On Friday, January 11, 2019 8:33:41 AM PST Jason Ekstrand wrote: >> > On Fri, Jan 11, 2019 at 10:19 AM Kenneth Graunke wrote: >> > > Those names (nir_var_func_local, nir_var_thread_local, and >> > > nir_var_thread_global) make more sense to me than private/function. >> > > >> > > Another option is `nir_var_local_temp` and `nir_var_shader_temp`, >> > > indicating that they're just temporary variables, and not anything >> > > with special semantics like memory. shader_temp would pair well with >> > > the existing shader_in/shader_out, since they have the same scope. >> > > >> > > I might also consider adding 'mem' to variables representing memory. >> > > >> > > So that would look like... >> > > >> > >nir_var_shader_in >> > >nir_var_shader_out >> > >nir_var_shader_temp (formerly local/function) >> > >nir_var_local_temp (formerly global/private) >> > > >> > >> > Are those flipped? >> >> Gah! Sorry. Yes. >> >>nir_var_shader_in >>nir_var_shader_out >>nir_var_shader_temp (formerly global/private) >>nir_var_local_temp (formerly local/function) >>nir_var_uniform >>nir_var_system_value >>nir_var_mem_ubo (added mem) >>nir_var_mem_ssbo (added mem) >>nir_var_mem_shared (added mem) >>nir_var_mem_global (the new global memory type being introduced) >> > > I can work with that. I do think I'd mildly prefer function_temp over > local_temp but I think the adding of _temp is an improvement. I like the _temp suggestion a lot! And I think I'm also mildly in favor of function_temp. (Also, thanks to taking naming seriously, to everyone involved here. It's hard.) signature.asc Description: PGP signature ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 05/12] nir: rename global/local to private/function memory
On Fri, Jan 11, 2019 at 11:11 AM Kenneth Graunke wrote: > On Friday, January 11, 2019 8:33:41 AM PST Jason Ekstrand wrote: > > On Fri, Jan 11, 2019 at 10:19 AM Kenneth Graunke wrote: > > > Those names (nir_var_func_local, nir_var_thread_local, and > > > nir_var_thread_global) make more sense to me than private/function. > > > > > > Another option is `nir_var_local_temp` and `nir_var_shader_temp`, > > > indicating that they're just temporary variables, and not anything > > > with special semantics like memory. shader_temp would pair well with > > > the existing shader_in/shader_out, since they have the same scope. > > > > > > I might also consider adding 'mem' to variables representing memory. > > > > > > So that would look like... > > > > > >nir_var_shader_in > > >nir_var_shader_out > > >nir_var_shader_temp (formerly local/function) > > >nir_var_local_temp (formerly global/private) > > > > > > > Are those flipped? > > Gah! Sorry. Yes. > >nir_var_shader_in >nir_var_shader_out >nir_var_shader_temp (formerly global/private) >nir_var_local_temp (formerly local/function) >nir_var_uniform >nir_var_system_value >nir_var_mem_ubo (added mem) >nir_var_mem_ssbo (added mem) >nir_var_mem_shared (added mem) >nir_var_mem_global (the new global memory type being introduced) > I can work with that. I do think I'd mildly prefer function_temp over local_temp but I think the adding of _temp is an improvement. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 05/12] nir: rename global/local to private/function memory
On Friday, January 11, 2019 8:33:41 AM PST Jason Ekstrand wrote: > On Fri, Jan 11, 2019 at 10:19 AM Kenneth Graunke wrote: > > Those names (nir_var_func_local, nir_var_thread_local, and > > nir_var_thread_global) make more sense to me than private/function. > > > > Another option is `nir_var_local_temp` and `nir_var_shader_temp`, > > indicating that they're just temporary variables, and not anything > > with special semantics like memory. shader_temp would pair well with > > the existing shader_in/shader_out, since they have the same scope. > > > > I might also consider adding 'mem' to variables representing memory. > > > > So that would look like... > > > >nir_var_shader_in > >nir_var_shader_out > >nir_var_shader_temp (formerly local/function) > >nir_var_local_temp (formerly global/private) > > > > Are those flipped? Gah! Sorry. Yes. nir_var_shader_in nir_var_shader_out nir_var_shader_temp (formerly global/private) nir_var_local_temp (formerly local/function) nir_var_uniform nir_var_system_value nir_var_mem_ubo (added mem) nir_var_mem_ssbo (added mem) nir_var_mem_shared (added mem) nir_var_mem_global (the new global memory type being introduced) --Ken signature.asc Description: This is a digitally signed message part. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 05/12] nir: rename global/local to private/function memory
" On Fri, Jan 11, 2019 at 5:19 PM Kenneth Graunke wrote: > > On Wednesday, January 9, 2019 5:33:22 PM PST Ian Romanick wrote: > > On 1/8/19 9:57 PM, Kenneth Graunke wrote: > > > On Tuesday, December 4, 2018 10:26:43 AM PST Karol Herbst wrote: > > >> the naming is a bit confusing no matter how you look at it. Within SPIR-V > > >> "global" memory is memory accessible from all threads. glsl "global" > > >> memory > > >> normally refers to shader thread private memory declared at global > > >> scope. As > > >> we already use "shared" for memory shared across all thrads of a work > > >> group > > >> the solution where everybody could be happy with is to rename "global" to > > >> "private" and use "global" later for memory usually stored within system > > >> accessible memory (be it VRAM or system RAM if keeping SVM in mind). > > >> glsl "local" memory is memory only accessible within a function, while > > >> SPIR-V > > >> "local" memory is memory accessible within the same workgroup. > > >> > > >> v2: rename local to function as well > > >> > > >> Signed-off-by: Karol Herbst > > > > > > I strongly dislike this patch, and I think we ought to revert it. > > > > > > This probably makes sense from an OpenCL memory-model view of the world, > > > but it's really confusing from a compiler or general programming point > > > of view. > > > > > > /Everybody/ knows what a local variable is. It's one of the most used > > > concepts in programming. Calling it nir_var_function is very confusing. > > > The variable is a...function? Maybe it's a function pointer? Neither > > > of those things even exist in GLSL, so...what the heck is it? > > > > > > Renaming global scope variables to "private" is also confusing IMO. > > > They're certainly not private to a function. They're globally > > > accessible by anything in the whole shader. I'll admit "global" isn't > > > a great name either. > > > > It seems like the concepts we're after a function local and thread > > local, so why not nir_var_thread_local (for old nir_var_global) and > > nir_var_function_local (for old nir_var_local). When "global" is > > reintroduced to mean thread global, we could add it as > > nir_var_thread_global. That seems to match at least one reasonable view > > of a storage hierarchy. > > Those names (nir_var_func_local, nir_var_thread_local, and > nir_var_thread_global) make more sense to me than private/function. > > Another option is `nir_var_local_temp` and `nir_var_shader_temp`, > indicating that they're just temporary variables, and not anything > with special semantics like memory. shader_temp would pair well with > the existing shader_in/shader_out, since they have the same scope. > > I might also consider adding 'mem' to variables representing memory. > > So that would look like... > >nir_var_shader_in >nir_var_shader_out >nir_var_shader_temp (formerly local/function) >nir_var_local_temp (formerly global/private) >nir_var_uniform >nir_var_system_value >nir_var_mem_ubo (added mem) >nir_var_mem_ssbo (added mem) >nir_var_mem_shared (added mem) >nir_var_mem_global (the new global memory type being introduced) > > How does that look? > overall it makes inherently sense. I just would like to express that "shader_temp" is actually memory belonging to a function (parameters or variables). > We may also want to rename the nir->globals list, or > nir_lower_global_vars_to_local and nir_opt_global_to_local. Not sure. > > --Ken ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 05/12] nir: rename global/local to private/function memory
On Fri, Jan 11, 2019 at 10:19 AM Kenneth Graunke wrote: > On Wednesday, January 9, 2019 5:33:22 PM PST Ian Romanick wrote: > > On 1/8/19 9:57 PM, Kenneth Graunke wrote: > > > On Tuesday, December 4, 2018 10:26:43 AM PST Karol Herbst wrote: > > >> the naming is a bit confusing no matter how you look at it. Within > SPIR-V > > >> "global" memory is memory accessible from all threads. glsl "global" > memory > > >> normally refers to shader thread private memory declared at global > scope. As > > >> we already use "shared" for memory shared across all thrads of a work > group > > >> the solution where everybody could be happy with is to rename > "global" to > > >> "private" and use "global" later for memory usually stored within > system > > >> accessible memory (be it VRAM or system RAM if keeping SVM in mind). > > >> glsl "local" memory is memory only accessible within a function, > while SPIR-V > > >> "local" memory is memory accessible within the same workgroup. > > >> > > >> v2: rename local to function as well > > >> > > >> Signed-off-by: Karol Herbst > > > > > > I strongly dislike this patch, and I think we ought to revert it. > > > > > > This probably makes sense from an OpenCL memory-model view of the > world, > > > but it's really confusing from a compiler or general programming point > > > of view. > > > > > > /Everybody/ knows what a local variable is. It's one of the most used > > > concepts in programming. Calling it nir_var_function is very > confusing. > > > The variable is a...function? Maybe it's a function pointer? Neither > > > of those things even exist in GLSL, so...what the heck is it? > > > > > > Renaming global scope variables to "private" is also confusing IMO. > > > They're certainly not private to a function. They're globally > > > accessible by anything in the whole shader. I'll admit "global" isn't > > > a great name either. > > > > It seems like the concepts we're after a function local and thread > > local, so why not nir_var_thread_local (for old nir_var_global) and > > nir_var_function_local (for old nir_var_local). When "global" is > > reintroduced to mean thread global, we could add it as > > nir_var_thread_global. That seems to match at least one reasonable view > > of a storage hierarchy. > > Those names (nir_var_func_local, nir_var_thread_local, and > nir_var_thread_global) make more sense to me than private/function. > > Another option is `nir_var_local_temp` and `nir_var_shader_temp`, > indicating that they're just temporary variables, and not anything > with special semantics like memory. shader_temp would pair well with > the existing shader_in/shader_out, since they have the same scope. > > I might also consider adding 'mem' to variables representing memory. > > So that would look like... > >nir_var_shader_in >nir_var_shader_out >nir_var_shader_temp (formerly local/function) >nir_var_local_temp (formerly global/private) > Are those flipped? >nir_var_uniform >nir_var_system_value >nir_var_mem_ubo (added mem) >nir_var_mem_ssbo (added mem) >nir_var_mem_shared (added mem) >nir_var_mem_global (the new global memory type being introduced) > > How does that look? > 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? > We may also want to rename the nir->globals list, or > nir_lower_global_vars_to_local and nir_opt_global_to_local. Not sure. > Yes, whatever we do, we should make those lists more consistent. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 05/12] nir: rename global/local to private/function memory
On Wednesday, January 9, 2019 5:33:22 PM PST Ian Romanick wrote: > On 1/8/19 9:57 PM, Kenneth Graunke wrote: > > On Tuesday, December 4, 2018 10:26:43 AM PST Karol Herbst wrote: > >> the naming is a bit confusing no matter how you look at it. Within SPIR-V > >> "global" memory is memory accessible from all threads. glsl "global" memory > >> normally refers to shader thread private memory declared at global scope. > >> As > >> we already use "shared" for memory shared across all thrads of a work group > >> the solution where everybody could be happy with is to rename "global" to > >> "private" and use "global" later for memory usually stored within system > >> accessible memory (be it VRAM or system RAM if keeping SVM in mind). > >> glsl "local" memory is memory only accessible within a function, while > >> SPIR-V > >> "local" memory is memory accessible within the same workgroup. > >> > >> v2: rename local to function as well > >> > >> Signed-off-by: Karol Herbst > > > > I strongly dislike this patch, and I think we ought to revert it. > > > > This probably makes sense from an OpenCL memory-model view of the world, > > but it's really confusing from a compiler or general programming point > > of view. > > > > /Everybody/ knows what a local variable is. It's one of the most used > > concepts in programming. Calling it nir_var_function is very confusing. > > The variable is a...function? Maybe it's a function pointer? Neither > > of those things even exist in GLSL, so...what the heck is it? > > > > Renaming global scope variables to "private" is also confusing IMO. > > They're certainly not private to a function. They're globally > > accessible by anything in the whole shader. I'll admit "global" isn't > > a great name either. > > It seems like the concepts we're after a function local and thread > local, so why not nir_var_thread_local (for old nir_var_global) and > nir_var_function_local (for old nir_var_local). When "global" is > reintroduced to mean thread global, we could add it as > nir_var_thread_global. That seems to match at least one reasonable view > of a storage hierarchy. Those names (nir_var_func_local, nir_var_thread_local, and nir_var_thread_global) make more sense to me than private/function. Another option is `nir_var_local_temp` and `nir_var_shader_temp`, indicating that they're just temporary variables, and not anything with special semantics like memory. shader_temp would pair well with the existing shader_in/shader_out, since they have the same scope. I might also consider adding 'mem' to variables representing memory. So that would look like... nir_var_shader_in nir_var_shader_out nir_var_shader_temp (formerly local/function) nir_var_local_temp (formerly global/private) nir_var_uniform nir_var_system_value nir_var_mem_ubo (added mem) nir_var_mem_ssbo (added mem) nir_var_mem_shared (added mem) nir_var_mem_global (the new global memory type being introduced) How does that look? We may also want to rename the nir->globals list, or nir_lower_global_vars_to_local and nir_opt_global_to_local. Not sure. --Ken signature.asc Description: This is a digitally signed message part. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 05/12] nir: rename global/local to private/function memory
On Thu, Jan 10, 2019 at 1:06 PM Karol Herbst wrote: > > On Thu, Jan 10, 2019 at 2:33 AM Ian Romanick wrote: > > > > On 1/8/19 9:57 PM, Kenneth Graunke wrote: > > > On Tuesday, December 4, 2018 10:26:43 AM PST Karol Herbst wrote: > > >> the naming is a bit confusing no matter how you look at it. Within SPIR-V > > >> "global" memory is memory accessible from all threads. glsl "global" > > >> memory > > >> normally refers to shader thread private memory declared at global > > >> scope. As > > >> we already use "shared" for memory shared across all thrads of a work > > >> group > > >> the solution where everybody could be happy with is to rename "global" to > > >> "private" and use "global" later for memory usually stored within system > > >> accessible memory (be it VRAM or system RAM if keeping SVM in mind). > > >> glsl "local" memory is memory only accessible within a function, while > > >> SPIR-V > > >> "local" memory is memory accessible within the same workgroup. > > >> > > >> v2: rename local to function as well > > >> > > >> Signed-off-by: Karol Herbst > > > > > > I strongly dislike this patch, and I think we ought to revert it. > > > > > > This probably makes sense from an OpenCL memory-model view of the world, > > > but it's really confusing from a compiler or general programming point > > > of view. > > > > > > /Everybody/ knows what a local variable is. It's one of the most used > > > concepts in programming. Calling it nir_var_function is very confusing. > > > The variable is a...function? Maybe it's a function pointer? Neither > > > of those things even exist in GLSL, so...what the heck is it? > > > > > > Renaming global scope variables to "private" is also confusing IMO. > > > They're certainly not private to a function. They're globally > > > accessible by anything in the whole shader. I'll admit "global" isn't > > > a great name either. > > > > It seems like the concepts we're after a function local and thread > > local, so why not nir_var_thread_local (for old nir_var_global) and > > nir_var_function_local (for old nir_var_local). When "global" is > > reintroduced to mean thread global, we could add it as > > nir_var_thread_global. That seems to match at least one reasonable view > > of a storage hierarchy. > > > > The old nir_var_global is > really something like thread_private memory accessible from everywhere > within the thread. > actually I was jumping between lines and the name "nir_var_thread_local" would actually fit. > > > I think we need to discuss this more. There are people with large > > > series of outstanding work that now have to rebase on this flag day > > > rename, and I don't think two people is enough consensus for renaming > > > a core IR concept. Can we find names we're all happy with? > > > > > > --Ken > > > > ___ > > mesa-dev mailing list > > mesa-dev@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 05/12] nir: rename global/local to private/function memory
On Thu, Jan 10, 2019 at 2:33 AM Ian Romanick wrote: > > On 1/8/19 9:57 PM, Kenneth Graunke wrote: > > On Tuesday, December 4, 2018 10:26:43 AM PST Karol Herbst wrote: > >> the naming is a bit confusing no matter how you look at it. Within SPIR-V > >> "global" memory is memory accessible from all threads. glsl "global" memory > >> normally refers to shader thread private memory declared at global scope. > >> As > >> we already use "shared" for memory shared across all thrads of a work group > >> the solution where everybody could be happy with is to rename "global" to > >> "private" and use "global" later for memory usually stored within system > >> accessible memory (be it VRAM or system RAM if keeping SVM in mind). > >> glsl "local" memory is memory only accessible within a function, while > >> SPIR-V > >> "local" memory is memory accessible within the same workgroup. > >> > >> v2: rename local to function as well > >> > >> Signed-off-by: Karol Herbst > > > > I strongly dislike this patch, and I think we ought to revert it. > > > > This probably makes sense from an OpenCL memory-model view of the world, > > but it's really confusing from a compiler or general programming point > > of view. > > > > /Everybody/ knows what a local variable is. It's one of the most used > > concepts in programming. Calling it nir_var_function is very confusing. > > The variable is a...function? Maybe it's a function pointer? Neither > > of those things even exist in GLSL, so...what the heck is it? > > > > Renaming global scope variables to "private" is also confusing IMO. > > They're certainly not private to a function. They're globally > > accessible by anything in the whole shader. I'll admit "global" isn't > > a great name either. > > It seems like the concepts we're after a function local and thread > local, so why not nir_var_thread_local (for old nir_var_global) and > nir_var_function_local (for old nir_var_local). When "global" is > reintroduced to mean thread global, we could add it as > nir_var_thread_global. That seems to match at least one reasonable view > of a storage hierarchy. > "function local" doesn't make that much sense, because only SPIR-V has a concept of function memory, which are usually just variables (usually stored as SSA values instead except you take an address of the variable and then it becomes function memory) and parameters where we end up using the old nir_var_local. The old nir_var_global is really something like thread_private memory accessible from everywhere within the thread. I don't really like the name "nir_var_thread_global" because what's that supposed to mean? Is it global from within the thread? Or are all threads able to access it? But then nir_var_global might still be better, because it's really memory accessible from basically everything. In worst case, it can also be accessed from various GPU engines or even a CPU even while the shader is still running (keeping SVM in mind). "nir_var_memory" might be also a possible name, but memory can be everything. In the end it is really something globally accessible by nearly everything. > > I think we need to discuss this more. There are people with large > > series of outstanding work that now have to rebase on this flag day > > rename, and I don't think two people is enough consensus for renaming > > a core IR concept. Can we find names we're all happy with? > > > > --Ken > > ___ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/mesa-dev ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 05/12] nir: rename global/local to private/function memory
On 1/8/19 9:57 PM, Kenneth Graunke wrote: > On Tuesday, December 4, 2018 10:26:43 AM PST Karol Herbst wrote: >> the naming is a bit confusing no matter how you look at it. Within SPIR-V >> "global" memory is memory accessible from all threads. glsl "global" memory >> normally refers to shader thread private memory declared at global scope. As >> we already use "shared" for memory shared across all thrads of a work group >> the solution where everybody could be happy with is to rename "global" to >> "private" and use "global" later for memory usually stored within system >> accessible memory (be it VRAM or system RAM if keeping SVM in mind). >> glsl "local" memory is memory only accessible within a function, while SPIR-V >> "local" memory is memory accessible within the same workgroup. >> >> v2: rename local to function as well >> >> Signed-off-by: Karol Herbst > > I strongly dislike this patch, and I think we ought to revert it. > > This probably makes sense from an OpenCL memory-model view of the world, > but it's really confusing from a compiler or general programming point > of view. > > /Everybody/ knows what a local variable is. It's one of the most used > concepts in programming. Calling it nir_var_function is very confusing. > The variable is a...function? Maybe it's a function pointer? Neither > of those things even exist in GLSL, so...what the heck is it? > > Renaming global scope variables to "private" is also confusing IMO. > They're certainly not private to a function. They're globally > accessible by anything in the whole shader. I'll admit "global" isn't > a great name either. It seems like the concepts we're after a function local and thread local, so why not nir_var_thread_local (for old nir_var_global) and nir_var_function_local (for old nir_var_local). When "global" is reintroduced to mean thread global, we could add it as nir_var_thread_global. That seems to match at least one reasonable view of a storage hierarchy. > I think we need to discuss this more. There are people with large > series of outstanding work that now have to rebase on this flag day > rename, and I don't think two people is enough consensus for renaming > a core IR concept. Can we find names we're all happy with? > > --Ken signature.asc Description: OpenPGP digital signature ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 05/12] nir: rename global/local to private/function memory
On Tue, Jan 8, 2019 at 11:57 PM Kenneth Graunke wrote: > On Tuesday, December 4, 2018 10:26:43 AM PST Karol Herbst wrote: > > the naming is a bit confusing no matter how you look at it. Within SPIR-V > > "global" memory is memory accessible from all threads. glsl "global" > memory > > normally refers to shader thread private memory declared at global > scope. As > > we already use "shared" for memory shared across all thrads of a work > group > > the solution where everybody could be happy with is to rename "global" to > > "private" and use "global" later for memory usually stored within system > > accessible memory (be it VRAM or system RAM if keeping SVM in mind). > > glsl "local" memory is memory only accessible within a function, while > SPIR-V > > "local" memory is memory accessible within the same workgroup. > > > > v2: rename local to function as well > > > > Signed-off-by: Karol Herbst > > I strongly dislike this patch, and I think we ought to revert it. > I'm fine with reverting it so that Matt can land stuff without the rebase pain. I thought it was just going to be one place and I tried to warn him about it but we probably should have held off a bit longer. > This probably makes sense from an OpenCL memory-model view of the world, > but it's really confusing from a compiler or general programming point > of view. > > /Everybody/ knows what a local variable is. It's one of the most used > concepts in programming. Calling it nir_var_function is very confusing. > The variable is a...function? Maybe it's a function pointer? Neither > of those things even exist in GLSL, so...what the heck is it? > Do they? Is it? I'm going to try to convince you that the suggested rename is at least not terrible and better than what we have today. We'll see if you buy it. :-) The old names made perfect sense in a OpenGL 3.0 world when you look at a single invocation of a shader. You have global variables and local variables just like you do in C. Then you have inputs, outputs, and uniforms which are magic globals which get pre-populated with data and/or data is read from them at the end of the execution; a bit weird, but okay. Since I/O is really just special globals, you really only have two scopes, local and global, and local < global in terms of who can access it. Ok, now step forward into 2019 with OpenGL 4.6, Vulkan, and compute. We now have "shared" variables and SSBOs which I'm going to, for the sake of momentary sanity, call "universal" because they are accessible from all threads in all stages in all workgroups. Now we have four scopes and they go universal > shared > global > local. Wait, shared > global? How does that make sense? It doesn't, really. If you take a step back and stop looking at your shader as if a single invocation is an entire program and instead look at is as a single invocation of a massively multi-threaded program, things start to make more sense. Things that we used to call "global" are no longer global in the C sense; they're now more of a thread-local storage. Compute shared variables aren't global either, they're a sort of storage that's somewhere between thread-local and universal which you could call workgroup-local. Hopefully, you can start to see where the new names come from. So what about function-local variables? Now that we've got three *-local things, local is getting a bit overloaded so we drop the "local" and call it "function". What about what we used to call "global" but which isn't really global anymore in the C sense? We could call it thread but that's a bit overloaded with SIMD since a thread may contain many invocations. We could call it "invocation" but that's a bit long and clumsy so we went with "private" because it's private in the sense that it's never touched by any other invocations. I'll grant that the name "private" is a bit weak but it's what SPIR-V used so we decided to not be too creative. If it helps, I hated the rename at first too but I've come to be ok with it. I'm not a huge fan of "function" and "private" but I don't really have anything better and I am firmly convinced that "global" doesn't make sense anymore. --Jason ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 05/12] nir: rename global/local to private/function memory
On Wed, Jan 9, 2019 at 6:57 AM Kenneth Graunke wrote: > > On Tuesday, December 4, 2018 10:26:43 AM PST Karol Herbst wrote: > > the naming is a bit confusing no matter how you look at it. Within SPIR-V > > "global" memory is memory accessible from all threads. glsl "global" memory > > normally refers to shader thread private memory declared at global scope. As > > we already use "shared" for memory shared across all thrads of a work group > > the solution where everybody could be happy with is to rename "global" to > > "private" and use "global" later for memory usually stored within system > > accessible memory (be it VRAM or system RAM if keeping SVM in mind). > > glsl "local" memory is memory only accessible within a function, while > > SPIR-V > > "local" memory is memory accessible within the same workgroup. > > > > v2: rename local to function as well > > > > Signed-off-by: Karol Herbst > > I strongly dislike this patch, and I think we ought to revert it. > > This probably makes sense from an OpenCL memory-model view of the world, > but it's really confusing from a compiler or general programming point > of view. > > /Everybody/ knows what a local variable is. It's one of the most used > concepts in programming. Calling it nir_var_function is very confusing. > The variable is a...function? Maybe it's a function pointer? Neither > of those things even exist in GLSL, so...what the heck is it? > > Renaming global scope variables to "private" is also confusing IMO. > They're certainly not private to a function. They're globally > accessible by anything in the whole shader. I'll admit "global" isn't > a great name either. > > I think we need to discuss this more. There are people with large > series of outstanding work that now have to rebase on this flag day > rename, and I don't think two people is enough consensus for renaming > a core IR concept. Can we find names we're all happy with? > > --Ken okay. I wouldn't mind that it gets reverted, the main idea behind it was, that I have another patch pending reintroducing "global" for global memory, which we need for kernels anyway and we wanted to run into those rebase issues exactly because of that. The main conflict here is that the current naming scheme is based on what the code sees, but that doesn't help us if we want to express what a thread sees. What is "global" in the former concept? ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 05/12] nir: rename global/local to private/function memory
On Tuesday, December 4, 2018 10:26:43 AM PST Karol Herbst wrote: > the naming is a bit confusing no matter how you look at it. Within SPIR-V > "global" memory is memory accessible from all threads. glsl "global" memory > normally refers to shader thread private memory declared at global scope. As > we already use "shared" for memory shared across all thrads of a work group > the solution where everybody could be happy with is to rename "global" to > "private" and use "global" later for memory usually stored within system > accessible memory (be it VRAM or system RAM if keeping SVM in mind). > glsl "local" memory is memory only accessible within a function, while SPIR-V > "local" memory is memory accessible within the same workgroup. > > v2: rename local to function as well > > Signed-off-by: Karol Herbst I strongly dislike this patch, and I think we ought to revert it. This probably makes sense from an OpenCL memory-model view of the world, but it's really confusing from a compiler or general programming point of view. /Everybody/ knows what a local variable is. It's one of the most used concepts in programming. Calling it nir_var_function is very confusing. The variable is a...function? Maybe it's a function pointer? Neither of those things even exist in GLSL, so...what the heck is it? Renaming global scope variables to "private" is also confusing IMO. They're certainly not private to a function. They're globally accessible by anything in the whole shader. I'll admit "global" isn't a great name either. I think we need to discuss this more. There are people with large series of outstanding work that now have to rebase on this flag day rename, and I don't think two people is enough consensus for renaming a core IR concept. Can we find names we're all happy with? --Ken signature.asc Description: This is a digitally signed message part. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 05/12] nir: rename global/local to private/function memory
Assuming it builds everywhere (probably best to double-check before pushing
anything)
Reviewed-by: Jason Ekstrand
On Tue, Dec 4, 2018 at 12:27 PM Karol Herbst wrote:
> the naming is a bit confusing no matter how you look at it. Within SPIR-V
> "global" memory is memory accessible from all threads. glsl "global" memory
> normally refers to shader thread private memory declared at global scope.
> As
> we already use "shared" for memory shared across all thrads of a work group
> the solution where everybody could be happy with is to rename "global" to
> "private" and use "global" later for memory usually stored within system
> accessible memory (be it VRAM or system RAM if keeping SVM in mind).
> glsl "local" memory is memory only accessible within a function, while
> SPIR-V
> "local" memory is memory accessible within the same workgroup.
>
> v2: rename local to function as well
>
> Signed-off-by: Karol Herbst
> ---
> src/amd/common/ac_nir_to_llvm.c | 6 ++--
> src/amd/vulkan/radv_shader.c | 8 ++---
> src/compiler/glsl/glsl_to_nir.cpp | 10 +++
> src/compiler/nir/nir.c| 6 ++--
> src/compiler/nir/nir.h| 8 ++---
> src/compiler/nir/nir_linking_helpers.c| 2 +-
> src/compiler/nir/nir_lower_clip.c | 2 +-
> .../nir/nir_lower_constant_initializers.c | 6 ++--
> .../nir/nir_lower_global_vars_to_local.c | 6 ++--
> .../nir/nir_lower_io_to_temporaries.c | 2 +-
> src/compiler/nir/nir_lower_locals_to_regs.c | 4 +--
> src/compiler/nir/nir_lower_vars_to_ssa.c | 8 ++---
> src/compiler/nir/nir_opt_copy_prop_vars.c | 8 ++---
> src/compiler/nir/nir_opt_dead_write_vars.c| 4 +--
> src/compiler/nir/nir_opt_find_array_copies.c | 4 +--
> src/compiler/nir/nir_opt_large_constants.c| 14 -
> src/compiler/nir/nir_print.c | 8 ++---
> src/compiler/nir/nir_remove_dead_variables.c | 6 ++--
> src/compiler/nir/nir_split_vars.c | 30 +--
> src/compiler/nir/nir_validate.c | 2 +-
> src/compiler/nir/tests/vars_tests.cpp | 18 +--
> src/compiler/spirv/vtn_cfg.c | 2 +-
> src/compiler/spirv/vtn_private.h | 2 +-
> src/compiler/spirv/vtn_variables.c| 8 ++---
> src/freedreno/ir3/ir3_nir.c | 2 +-
> src/gallium/auxiliary/nir/tgsi_to_nir.c | 2 +-
> src/gallium/drivers/v3d/v3d_program.c | 2 +-
> src/gallium/drivers/vc4/vc4_program.c | 4 +--
> src/intel/compiler/brw_nir.c | 10 +++
> src/intel/vulkan/anv_pipeline.c | 4 +--
> src/mesa/main/glspirv.c | 2 +-
> src/mesa/state_tracker/st_glsl_to_nir.cpp | 4 +--
> 32 files changed, 102 insertions(+), 102 deletions(-)
>
> diff --git a/src/amd/common/ac_nir_to_llvm.c
> b/src/amd/common/ac_nir_to_llvm.c
> index 18e9b69f3c0..2d8a27a0ab9 100644
> --- a/src/amd/common/ac_nir_to_llvm.c
> +++ b/src/amd/common/ac_nir_to_llvm.c
> @@ -1923,7 +1923,7 @@ static LLVMValueRef visit_load_var(struct
> ac_nir_context *ctx,
> values[chan] = ctx->abi->inputs[idx + chan
> + const_index * stride];
> }
> break;
> - case nir_var_local:
> + case nir_var_function:
> for (unsigned chan = 0; chan < ve; chan++) {
> if (indir_index) {
> unsigned count =
> glsl_count_attribute_slots(
> @@ -2055,7 +2055,7 @@ visit_store_var(struct ac_nir_context *ctx,
> }
> }
> break;
> - case nir_var_local:
> + case nir_var_function:
> for (unsigned chan = 0; chan < 8; chan++) {
> if (!(writemask & (1 << chan)))
> continue;
> @@ -4061,7 +4061,7 @@ ac_lower_indirect_derefs(struct nir_shader *nir,
> enum chip_class chip_class)
> * See the following thread for more details of the problem:
> *
> https://lists.freedesktop.org/archives/mesa-dev/2017-July/162106.html
> */
> - indirect_mask |= nir_var_local;
> + indirect_mask |= nir_var_function;
>
> nir_lower_indirect_derefs(nir, indirect_mask);
> }
> diff --git a/src/amd/vulkan/radv_shader.c b/src/amd/vulkan/radv_shader.c
> index 456c462a230..fa15478ad2d 100644
> --- a/src/amd/vulkan/radv_shader.c
> +++ b/src/amd/vulkan/radv_shader.c
> @@ -126,8 +126,8 @@ radv_optimize_nir(struct nir_shader *shader, bool
> optimize_conservatively,
> do {
> progress = false;
>
> - NIR_PASS(progress, shader, nir_split_array_vars,
> nir_var_local);
> - NIR_PASS(progress, shader, nir_shrink_vec_array_vars,
> nir_var_local);
> + NIR_PASS(progress, shader, nir_split_array_vars,
> nir_var_function);
> +
