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

[Ilia Mirkin]

On Thu, Mar 31, 2016 at 5:07 AM, Alejandro Piñeiro <apinheiro at igalia.com> wrote:
> 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)

My original thought was that it'd be better to just have a computation
in the shader once rather than every time it's used and then relying
on CSE to figure it all out. We should just teach the warning pass to
ignore is_gl_identifier() and move on. Of course it's not a ton of
code, so if you guys desperately want to duplicate this stuff in nir,
I'm sure it'd be quite easy to throw something into glsl_to_tgsi to
account for it.

  -ilia