[Mesa-dev] [PATCH 1/7] glsl: move gl_shader_stage_name() definition to shader_enums.h

[Rob Clark]

On Thu, Oct 8, 2015 at 6:29 PM, Ilia Mirkin <imirkin at alum.mit.edu> wrote:
> On Thu, Oct 8, 2015 at 3:57 PM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
>> On 8 October 2015 at 19:38, Ilia Mirkin <imirkin at alum.mit.edu> wrote:
>>> On Thu, Oct 8, 2015 at 2:34 PM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
>>>> On 8 October 2015 at 19:32, Ilia Mirkin <imirkin at alum.mit.edu> wrote:
>>>>> On Thu, Oct 8, 2015 at 2:23 PM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
>>>>>> On 8 October 2015 at 18:57, Ilia Mirkin <imirkin at alum.mit.edu> wrote:
>>>>>>> On Thu, Oct 8, 2015 at 1:52 PM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
>>>>>>>> On 8 October 2015 at 18:08, Ilia Mirkin <imirkin at alum.mit.edu> wrote:
>>>>>>>>> On Thu, Oct 8, 2015 at 1:09 PM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
>>>>>>>>>> This is a trivial enough function that can live in the header. While
>>>>>>>>>> we're here, add a STATIC_ASSERT for good measure.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Emil Velikov <emil.velikov at collabora.com>
>>>>>>>>>> ---
>>>>>>>>>>  src/glsl/shader_enums.c | 13 -------------
>>>>>>>>>>  src/glsl/shader_enums.h | 23 +++++++++++++++++++++--
>>>>>>>>>>  2 files changed, 21 insertions(+), 15 deletions(-)
>>>>>>>>>>
>>>>>>>>>> diff --git a/src/glsl/shader_enums.c b/src/glsl/shader_enums.c
>>>>>>>>>> index c196b79..b3da3e9 100644
>>>>>>>>>> --- a/src/glsl/shader_enums.c
>>>>>>>>>> +++ b/src/glsl/shader_enums.c
>>>>>>>>>> @@ -32,19 +32,6 @@
>>>>>>>>>>  #define ENUM(x) [x] = #x
>>>>>>>>>>  #define NAME(val) ((((val) < ARRAY_SIZE(names)) && names[(val)]) ? names[(val)] : "UNKNOWN")
>>>>>>>>>>
>>>>>>>>>> -const char * gl_shader_stage_name(gl_shader_stage stage)
>>>>>>>>>> -{
>>>>>>>>>> -   static const char *names[] = {
>>>>>>>>>> -      ENUM(MESA_SHADER_VERTEX),
>>>>>>>>>> -      ENUM(MESA_SHADER_TESS_CTRL),
>>>>>>>>>> -      ENUM(MESA_SHADER_TESS_EVAL),
>>>>>>>>>> -      ENUM(MESA_SHADER_GEOMETRY),
>>>>>>>>>> -      ENUM(MESA_SHADER_FRAGMENT),
>>>>>>>>>> -      ENUM(MESA_SHADER_COMPUTE),
>>>>>>>>>> -   };
>>>>>>>>>> -   return NAME(stage);
>>>>>>>>>> -}
>>>>>>>>>> -
>>>>>>>>>>  const char * gl_vert_attrib_name(gl_vert_attrib attrib)
>>>>>>>>>>  {
>>>>>>>>>>     static const char *names[] = {
>>>>>>>>>> diff --git a/src/glsl/shader_enums.h b/src/glsl/shader_enums.h
>>>>>>>>>> index 2a5d2c5..fbd2744 100644
>>>>>>>>>> --- a/src/glsl/shader_enums.h
>>>>>>>>>> +++ b/src/glsl/shader_enums.h
>>>>>>>>>> @@ -43,10 +43,27 @@ typedef enum
>>>>>>>>>>     MESA_SHADER_COMPUTE = 5,
>>>>>>>>>>  } gl_shader_stage;
>>>>>>>>>>
>>>>>>>>>> -const char * gl_shader_stage_name(gl_shader_stage stage);
>>>>>>>>>> -
>>>>>>>>>>  #define MESA_SHADER_STAGES (MESA_SHADER_COMPUTE + 1)
>>>>>>>>>>
>>>>>>>>>> +#define ENUM(x) [x] = #x
>>>>>>>>>> +
>>>>>>>>>> +static const char *gl_shader_stage_names[] = {
>>>>>>>>>> +   ENUM(MESA_SHADER_VERTEX),
>>>>>>>>>> +   ENUM(MESA_SHADER_TESS_CTRL),
>>>>>>>>>> +   ENUM(MESA_SHADER_TESS_EVAL),
>>>>>>>>>> +   ENUM(MESA_SHADER_GEOMETRY),
>>>>>>>>>> +   ENUM(MESA_SHADER_FRAGMENT),
>>>>>>>>>> +   ENUM(MESA_SHADER_COMPUTE),
>>>>>>>>>> +};
>>>>>>>>>
>>>>>>>>> Won't this bloat the binary by including this in every object file? I
>>>>>>>>> don't know that the array will get merged. (The strings will though.)
>>>>>>>>> Normally these things live in c files so that they're stored in only
>>>>>>>>> one object.
>>>>>>>>>
>>>>>>>> The linker (with its garbage collector) will sort it out for us. Seems
>>>>>>>> like we do get a small increase from the series
>>>>>>>>
>>>>>>>>    text    data     bss     dec     hex filename
>>>>>>>> 5103374  204048   27648 5335070  51681e i965_dri.so.after
>>>>>>>> 5104590  204208   27648 5336446  516d7e i965_dri.so.before
>>>>>>>>
>>>>>>>> Namely - we gain ~1.2k of text. Which imho doesn't sound that bad ?
>>>>>>>
>>>>>>> In exchange for what? From what I can tell... nothing. The array
>>>>>>> definition needs to live in a c file. You can move the function to
>>>>>>> look up from it into a header, that's fine.
>>>>>>
>>>>>> Nasty bug in(?) autohell [1]. If you're happy to dive in the auto*
>>>>>> boat and fix it, I would gladly purge this series :-)
>>>>>
>>>>> Well that's just asking for trouble:
>>>>>
>>>>> src/Makefile.am:        glsl/shader_enums.c \
>>>>> src/mesa/Makefile.sources:      ../glsl/shader_enums.c \
>>>>>
>>>> Yes it is nasty.
>>>>
>>>>> It should just be removed from shader_enums.c and make sure that
>>>>> libglsl_util is a dep of libmesa_la and libmesagallium_la. Am I
>>>>> missing something?
>>>> Give it a go and admire the duplicated symbols :)
>>>
>>> Errr, that was a typo. I meant it should be removed from
>>> PROGRAM_FILES. Any additional duplicated items should be removed as
>>> well.
>> Patches welcome :-) And please give the scons a test as well, just in case.
>
> Looks like Rob just sent one. Good thing I was lazy about looking into it.


yeah.. I'd like to unwind the dependency on glsl_types too, but I
figure that could wait so I sent the 'low hanging' part of just moving
shader_enums into nir..

BR,
-R