On 31/03/16 09:37, Jordan Justen wrote: > On 2016-03-30 02:03:29, Alejandro Piñeiro wrote: >> On 30/03/16 09:44, Alejandro Piñeiro wrote: >>> On 30/03/16 06:16, Kenneth Graunke wrote: >>>> I was just looking at that today...adding an ordinary global variable >>>> in builtin_variables.cpp seems really wrong. >>> I think that I don't follow. Why gl_GlobalInvocationID is an ordinary >>> global variable? It is a built-in variable, and I assumed that all >>> built-ins would be using the ir_var_system_value ir_variable_mode. As >>> far as I understand this is not happening here due the lowering. >> Ah ok, I think that I understand what you mean, now that I took a look >> to builtin_variables.cpp::1171. Yes gl_GlobalInvocationID and >> gl_LocalInvocationIndex are not added with add_system_value. As far as I >> see, it is because they are not included as system values at >> shader_enums.h:gl_system_value. I guess that because it is lowered as >> Illia mentioned. Jordan could you confirm the reason of adding those >> built-ins as regular variables? >> > In v1, they were system values: > > http://thread.gmane.org/gmane.comp.video.mesa3d.devel/109477 > > I think based on Ilia's feedback: > > " have you considered just starting out *every* CS with the > instructions to compute them and then not having any lowering at > all? " > > I tried to handle the initialization of the variables differently. I > don't think Ilia's feedback really commented on if it was a system > value or not, so dropping the system value type was my idea/fault. > > In v2, instead having a lowering pass that replaces the system value, > I inserted some code into main to initialize the variable. > > http://thread.gmane.org/gmane.comp.video.mesa3d.devel/110920
Thanks for the summary. > I happen to think the glsl ir based init code looks better than the > system value lowering pass, but if we have some rule that all gl_* > variables must be system values, then I guess it was a bad choice. Well, in my opinion, it makes sense and sounds natural to mark all built-in variables as system values. Having said so, as far as I see only this "uninitialized variable" warning that I just added was being affected, that is a nice enhancement, but not a core functionality. So not sure if makes sense to change all that code now (more about this below) just for this warning. > I didn't think that this variable (and gl_LocalInvocationIndex) had a > real need to be turned into system values until there was a driver > that had a special way to initialize the variables which was better > than the result from the glsl ir based initialization. > > Finally, it appears that Jason is adding these as a system value for > spir-v, which then gets lowered at the nir level. Well, this raises a good point. Right now nir lacks too the system values tied to those two built-ins. So for example, just trying to mark them as system values on glsl triggers an assertion later on nir. > I think it would be > a reasonable idea to convert it to a system value, and not lower it in > glsl at all. This would then require the non-nir drivers to lower it > elsewhere. (In fact, this was suggested by Jason in v1 of the series, > but Ilia wanted it handled at the glsl level.) Taking into account that some people are already working on modifying those system values, I don't think that it make sense myself doing the same work for fixing the false positives of this warning. So in conclusion, I think that at this point it would be better (and easier) to just filter out those built-in by name when the warning is being raised. We can revisit that code if those built-in are converted to system values in the future. Opinions? (CCing Jason just in case he wants to share his opinion too) BR _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
