[Mesa-dev] [PATCH v2 2/2] glsl: raise warning when using uninitialized variables

[Alejandro Piñeiro]

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