[Mesa-dev] [PATCH 1/2] st/mesa: use enum types instead of int/unsigned

[Brian Paul]

On 11/07/2017 07:20 AM, Eric Engestrom wrote:
> On Tuesday, 2017-11-07 07:07:55 -0700, Brian Paul wrote:
>> On 11/07/2017 03:36 AM, Eric Engestrom wrote:
>>> On Monday, 2017-11-06 14:00:30 -0700, Brian Paul wrote:
>>>> Use the proper enum types for various variables.  Makes life in gdb
>>>> a little nicer.
>>>> ---
>>>>    src/mesa/state_tracker/st_glsl_to_tgsi.cpp       | 7 ++++---
>>>>    src/mesa/state_tracker/st_glsl_to_tgsi_private.h | 6 +++---
>>>>    src/mesa/state_tracker/st_mesa_to_tgsi.c         | 6 +++---
>>>>    src/mesa/state_tracker/st_mesa_to_tgsi.h         | 7 ++++---
>>>>    4 files changed, 14 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/src/mesa/state_tracker/st_glsl_to_tgsi.cpp b/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
>>>> index 54e1961..2048b59 100644
>>>> --- a/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
>>>> +++ b/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
>>>> @@ -179,10 +179,10 @@ public:
>>>>       int num_address_regs;
>>>>       uint32_t samplers_used;
>>>>       glsl_base_type sampler_types[PIPE_MAX_SAMPLERS];
>>>> -   int sampler_targets[PIPE_MAX_SAMPLERS];   /**< One of TGSI_TEXTURE_* */
>>>> +   enum tgsi_texture_type sampler_targets[PIPE_MAX_SAMPLERS];
>>>>       int images_used;
>>>>       int image_targets[PIPE_MAX_SHADER_IMAGES];
>>>> -   unsigned image_formats[PIPE_MAX_SHADER_IMAGES];
>>>> +   enum pipe_format image_formats[PIPE_MAX_SHADER_IMAGES];
>>>>       bool indirect_addr_consts;
>>>>       int wpos_transform_const;
>>>>
>>>> @@ -6489,7 +6489,8 @@ st_translate_program(
>>>>       /* texture samplers */
>>>>       for (i = 0; i < frag_const->MaxTextureImageUnits; i++) {
>>>>          if (program->samplers_used & (1u << i)) {
>>>> -         unsigned type = st_translate_texture_type(program->sampler_types[i]);
>>>> +         enum tgsi_return_type type =
>>>> +            st_translate_texture_type(program->sampler_types[i]);
>>>>
>>>>             t->samplers[i] = ureg_DECL_sampler(ureg, i);
>>>>
>>>> diff --git a/src/mesa/state_tracker/st_glsl_to_tgsi_private.h b/src/mesa/state_tracker/st_glsl_to_tgsi_private.h
>>>> index d57525d..bdc7448 100644
>>>> --- a/src/mesa/state_tracker/st_glsl_to_tgsi_private.h
>>>> +++ b/src/mesa/state_tracker/st_glsl_to_tgsi_private.h
>>>> @@ -127,13 +127,13 @@ public:
>>>>       unsigned is_64bit_expanded:1;
>>>>       unsigned sampler_base:5;
>>>>       unsigned sampler_array_size:6; /**< 1-based size of sampler array, 1 if not array */
>>>> -   unsigned tex_target:4; /**< One of TEXTURE_*_INDEX */
>>>> +   gl_texture_index tex_target:5;
>>>
>>> This bump is an unrelated bug fix, which should be in its own commit
>>> (before this patch) with:
>>
>> Mmm, it's not really a bug fix.  The 4-bit unsigned field is fine as it is.
>> But when we change an unsigned bitfield into a enum bitfield we have add one
>> extra bit for MSVC to ensure the high bit is always zero. See
>> eedecb4ecae36c5b019 for more info.
>
> I was thinking of the fact there are 12 values in the enum, but my maths
> was off, 4 bits are enough for that.
>
> Worth adding the MSVC comment to the commit message?

Sure.

>
>>
>> There should be no change in the struct size.
>>
>> -Brian
>>
>>>
>>> Cc: Marek Olšák <marek.olsak at amd.com>
>>> Fixes: dbf64ea28bb20bafe5a7d "glsl_to_tgsi: reduce the size of
>>>          glsl_to_tgsi_instruction using bitfields"
>>> Reviewed-by: Eric Engestrom <eric.engestrom at imgtec.com>
>>>
>>>>       glsl_base_type tex_type:5;
>>>>       unsigned tex_shadow:1;
>>>> -   unsigned image_format:9;
>>>> +   enum pipe_format image_format:9;
>
> Values up to 310 => 9 bits, so this should be changed to 10 to account for
> the MSVC "peculiarity", right?

Yes!  Thanks.  v2 coming.

-Brian

>
>>>>       unsigned tex_offset_num_offset:3;
>>>>       unsigned dead_mask:4; /**< Used in dead code elimination */
>>>> -   unsigned buffer_access:3; /**< buffer access type */
>>>> +   unsigned buffer_access:3; /**< bitmask of TGSI_MEMORY_x bits */
>>>>
>>>>       const struct tgsi_opcode_info *info;
>>>>    };
>>>> diff --git a/src/mesa/state_tracker/st_mesa_to_tgsi.c b/src/mesa/state_tracker/st_mesa_to_tgsi.c
>>>> index fa9fa44..8a61776 100644
>>>> --- a/src/mesa/state_tracker/st_mesa_to_tgsi.c
>>>> +++ b/src/mesa/state_tracker/st_mesa_to_tgsi.c
>>>> @@ -166,8 +166,8 @@ src_register( struct st_translate *t,
>>>>    /**
>>>>     * Map mesa texture target to TGSI texture target.
>>>>     */
>>>> -unsigned
>>>> -st_translate_texture_target(GLuint textarget, GLboolean shadow)
>>>> +enum tgsi_texture_type
>>>> +st_translate_texture_target(gl_texture_index textarget, GLboolean shadow)
>>>>    {
>>>>       if (shadow) {
>>>>          switch (textarget) {
>>>> @@ -225,7 +225,7 @@ st_translate_texture_target(GLuint textarget, GLboolean shadow)
>>>>    /**
>>>>     * Map GLSL base type to TGSI return type.
>>>>     */
>>>> -unsigned
>>>> +enum tgsi_return_type
>>>>    st_translate_texture_type(enum glsl_base_type type)
>>>>    {
>>>>    	switch (type) {
>>>> diff --git a/src/mesa/state_tracker/st_mesa_to_tgsi.h b/src/mesa/state_tracker/st_mesa_to_tgsi.h
>>>> index 106cf85..06e8b70 100644
>>>> --- a/src/mesa/state_tracker/st_mesa_to_tgsi.h
>>>> +++ b/src/mesa/state_tracker/st_mesa_to_tgsi.h
>>>> @@ -30,6 +30,7 @@
>>>>    #define ST_MESA_TO_TGSI_H
>>>>
>>>>    #include "main/glheader.h"
>>>> +#include "main/mtypes.h"
>>>>
>>>>    #include "pipe/p_compiler.h"
>>>>    #include "pipe/p_defines.h"
>>>> @@ -62,10 +63,10 @@ st_translate_mesa_program(
>>>>       const ubyte outputSemanticName[],
>>>>       const ubyte outputSemanticIndex[]);
>>>>
>>>> -unsigned
>>>> -st_translate_texture_target(GLuint textarget, GLboolean shadow);
>>>> +enum tgsi_texture_type
>>>> +st_translate_texture_target(gl_texture_index textarget, GLboolean shadow);
>>>>
>>>> -unsigned
>>>> +enum tgsi_return_type
>>>>    st_translate_texture_type(enum glsl_base_type type);
>>>>
>>>>    #if defined __cplusplus
>>>> --
>>>> 1.9.1
>>>>
>>