[Mesa-dev] [PATCH 1/3] glsl: add gl_HelperInvocation system value

[Tapani Pälli]

On 11/10/2015 01:06 PM, Ilia Mirkin wrote:
> On Tue, Sep 15, 2015 at 12:51 AM, Ilia Mirkin <imirkin at alum.mit.edu> wrote:
>> On Tue, Sep 15, 2015 at 12:41 AM, Tapani Pälli <tapani.palli at intel.com> wrote:
>>>
>>>
>>> On 09/15/2015 12:43 AM, Ilia Mirkin wrote:
>>>>
>>>> On Mon, Sep 14, 2015 at 4:35 PM, Ilia Mirkin <imirkin at alum.mit.edu> wrote:
>>>>>
>>>>> Signed-off-by: Ilia Mirkin <imirkin at alum.mit.edu>
>>>>> ---
>>>>>    src/glsl/builtin_variables.cpp             | 3 +++
>>>>>    src/glsl/shader_enums.h                    | 1 +
>>>>>    src/mesa/state_tracker/st_glsl_to_tgsi.cpp | 1 +
>>>>>    3 files changed, 5 insertions(+)
>>>>>
>>>>> diff --git a/src/glsl/builtin_variables.cpp
>>>>> b/src/glsl/builtin_variables.cpp
>>>>> index b5e2908..7ea5db8 100644
>>>>> --- a/src/glsl/builtin_variables.cpp
>>>>> +++ b/src/glsl/builtin_variables.cpp
>>>>> @@ -1050,6 +1050,9 @@
>>>>> builtin_variable_generator::generate_fs_special_vars()
>>>>>          add_input(VARYING_SLOT_LAYER, int_t, "gl_Layer");
>>>>>          add_input(VARYING_SLOT_VIEWPORT, int_t, "gl_ViewportIndex");
>>>>>       }
>>>>> +
>>>>> +   if (state->is_version(450, 310))
>>>>
>>>>
>>>> Hmmm... this should probably be
>>>>
>>>> state->ARB_ES3_1_compatibility_enable || state->is_version(450, 310).
>>>>
>>>> Since that doesn't exist, perhaps i should just add it in /* ... */ to
>>>> be uncommented later? Or I guess I could go in and add all of those
>>>> ext enables in. What do people think? It's easy enough to do any of
>>>> these.
>>>
>>>
>>> IMO comments would be enough for now, we can worry about ES3_1_compatibility
>>> when we have 3.1 done.
>>
>> OK, that was my feeling as well.
>>
>>>
>>>>> +      add_system_value(SYSTEM_VALUE_FRAG_HELPER, bool_t,
>>>>> "gl_HelperInvocation");
>>>>>    }
>>>>>
>>>>>
>>>>> diff --git a/src/glsl/shader_enums.h b/src/glsl/shader_enums.h
>>>>> index 7c598b6..6bc93a5 100644
>>>>> --- a/src/glsl/shader_enums.h
>>>>> +++ b/src/glsl/shader_enums.h
>>>>> @@ -352,6 +352,7 @@ typedef enum
>>>>>       SYSTEM_VALUE_SAMPLE_ID,
>>>>>       SYSTEM_VALUE_SAMPLE_POS,
>>>>>       SYSTEM_VALUE_SAMPLE_MASK_IN,
>>>>> +   SYSTEM_VALUE_FRAG_HELPER,
>>>
>>>
>>> Why not call it SYSTEM_VALUE_HELPER_INVOCATION? Most builtin variables are
>>> named with matching names to spec.
>>
>> I didn't want something so unnecessarily long. Here are some counterexamples:
>>
>> ....
>>
>> I could have sworn there were some, but apparently not :) The
>> VARYING_SLOT_* are a lot more counter-example-ish, like
>> VARYING_SLOT_POS, VARYING_SLOT_FACE, VARYING_SLOT_PNTC, etc.
>>
>> Anyone else have thoughts on how these should be named?
>
> ES 3.1 appears to be getting closer to reality... probably good to
> close this one out... Reviews/opinions welcome.
>

I vote again for using the exact name, it'll be easier to understand 
from the code later. You have my r-b with that change!

// Tapani