On 10 October 2015 at 01:56, Rob Clark <robdcl...@gmail.com> wrote: > On Fri, Oct 9, 2015 at 8:07 PM, Emil Velikov <emil.l.veli...@gmail.com> wrote: >> On 9 October 2015 at 23:27, Rob Clark <robdcl...@gmail.com> wrote: >>> On Fri, Oct 9, 2015 at 5:57 PM, Emil Velikov <emil.l.veli...@gmail.com> >>> wrote: >>>> On 9 October 2015 at 21:31, Rob Clark <robdcl...@gmail.com> wrote: >>>>> From: Rob Clark <robcl...@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.veli...@gmail.com> >>>>> Signed-off-by: Rob Clark <robcl...@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 _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
