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

[Timothy Arceri]

On 10 November 2015 10:06:49 pm AEDT, Ilia Mirkin <imirkin at alum.mit.edu> 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 like using the full name too. I guess you could always change these to SYS_VAL_* then its only one char bigger than your suggestion, not sure how big of a change that would be.


>_______________________________________________
>mesa-dev mailing list
>mesa-dev at lists.freedesktop.org
>http://lists.freedesktop.org/mailman/listinfo/mesa-dev