[Mesa-dev] [PATCH] glsl: couple shader_enums cleanups

[Emil Velikov]

On 10 October 2015 at 01:56, Rob Clark <robdclark at gmail.com> wrote:
> On Fri, Oct 9, 2015 at 8:07 PM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
>> On 9 October 2015 at 23:27, Rob Clark <robdclark at gmail.com> wrote:
>>> On Fri, Oct 9, 2015 at 5:57 PM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
>>>> On 9 October 2015 at 21:31, Rob Clark <robdclark at gmail.com> wrote:
>>>>> From: Rob Clark <robclark at freedesktop.org>
>>>>>
>>>>> Add missing enum to gl_system_value_name() and move VARYING_SLOT_MAX /
>>>>> FRAG_RESULT_MAX / etc into shader_enums.h as suggested by Emil.
>>>>>
>>>>> Reported-by: Emil Velikov <emil.l.velikov at gmail.com>
>>>>> Signed-off-by: Rob Clark <robclark at freedesktop.org>
>>>>> ---
>>>>> Note: punted on the STATIC_ASSERT() thing for now..  kinda wanted some-
>>>>> think more like tgsi_strings_check() where we check everything once on
>>>>> startup, so you don't have to trigger something that calls the various
>>>>> xyz_name() to realize something is out of sync.
>>>>>
>>>> I'm confused - isn't static_assert a compile thing ?
>>>
>>> oh, heh, good point..
>>>
>>>>>  src/glsl/nir/shader_enums.c | 1 +
>>>>>  src/glsl/nir/shader_enums.h | 7 +++++++
>>>>>  src/mesa/main/mtypes.h      | 5 -----
>>>>>  3 files changed, 8 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/src/glsl/nir/shader_enums.c b/src/glsl/nir/shader_enums.c
>>>>> index 3722475..7fcbe81 100644
>>>>> --- a/src/glsl/nir/shader_enums.c
>>>>> +++ b/src/glsl/nir/shader_enums.c
>>>>> @@ -169,6 +169,7 @@ const char * gl_system_value_name(gl_system_value sysval)
>>>>>       ENUM(SYSTEM_VALUE_TESS_LEVEL_INNER),
>>>>>       ENUM(SYSTEM_VALUE_LOCAL_INVOCATION_ID),
>>>>>       ENUM(SYSTEM_VALUE_WORK_GROUP_ID),
>>>>> +     ENUM(SYSTEM_VALUE_NUM_WORK_GROUPS),
>>>>>       ENUM(SYSTEM_VALUE_VERTEX_CNT),
>>>>>     };
>>>>>     return NAME(sysval);
>>>>> diff --git a/src/glsl/nir/shader_enums.h b/src/glsl/nir/shader_enums.h
>>>>> index 2a5d2c5..77638ba 100644
>>>>> --- a/src/glsl/nir/shader_enums.h
>>>>> +++ b/src/glsl/nir/shader_enums.h
>>>>> @@ -233,6 +233,11 @@ typedef enum
>>>>>     VARYING_SLOT_VAR31,
>>>>>  } gl_varying_slot;
>>>>>
>>>>> +
>>>>> +#define VARYING_SLOT_MAX       (VARYING_SLOT_VAR0 + MAX_VARYING)
>>>> I'd keep this and FRAG_RESULT_MAX defined as FOO + 1. Otherwise we'll
>>>> end in funny spaghetti of header dependencies due to the extra two
>>>> macros.
>>>
>>> not quite sure, but it sounds like you are asking to open-code
>>> MAX_VARYINGS?  I don't think that is needed since that seems to
>>> basically be a global config.h type thing (I mean mesa/main/config.h,
>>> not autoconf one)
>>>
>>> There isn't really *supposed* to be a VARYING_SLOT_VAR(n-1) (ignoring
>>> the fact that I added them simply to get nice string values to print
>>> out in nir_print)
>>>
>> I was thinking that #define VARYING_SLOT_MAX (VARYING_SLOT_VAR + 1)
>> /*VARYING_SLOT_VAR(n) */ is the preferred way.
>> Hmm can you please point me in the right direction, as to why having
>> VARYING_SLOT_VAR(n-1) is a bad idea ?
>
> A few of the enums are meant to have their last element be interpreted
> as xyz0 + n (where n < #define MAX_xyz from config.h)
>
> I guess technically the max isn't something that is likely to change
> frequently but if we ignore the MAX_xyz from config.h that means we
> have two places to update if the max was ever increased.
>
If we go for the 0 + MAX_foo approach, we'll get some lovely build
warnings. On the other hand things will break nicely with +1. I'm
slightly leaning towards the latter, as it seems that currently things
are subtle/brittle and by breaking them (albeit unlikely in the near
future) we'll get a victim^Wvolunteer to untangle/make things obvious.

Just my 2c. I'm not proposing that you have to do anything on the
topic, neither am I volunteering.

>>>>> +#define VARYING_SLOT_PATCH0    (VARYING_SLOT_MAX)
>>>>> +#define VARYING_SLOT_TESS_MAX  (VARYING_SLOT_PATCH0 + MAX_VARYING)
>>>>> +
>>>> As there are defined 'on top'/'after' of the existing enum perhaps we
>>>> should keep them alongside their PRIM_MAX...UNKNOWN brethren ?
>>>
>>> PRIM_MAX is a different thing, not related to anything that is already
>>> in shader_enums..
>>>
>> I had in mind the relationship wrt the respective enums/macros, rather
>> than their meaning. Although with the _MAX above in mind, I'm likely
>> missing something.
>
> prim == gl draw primitive (ie. tris/tristrip/quads/points/etc)..  not
> something that is internal to mesa or in shader_enums.h..
>
That's precisely what I meant. PRIM_MAX and VARYING_SLOT_MAX are well
defined quantities with the former unlikey to ever change.
VARYING_SLOT_PATCH0 and VARYING_SLOT_TESS_MAX on the other hand, are
'duck taped' on top of the existing gl_varying_slot enum. Similar to
PRIM_OUTSIDE_BEGIN_END and PRIM_UNKNOWN.

Cheers,
Emil