[Mesa-dev] [PATCH] st/mesa: use enum types instead of int/unsigned (v2)

[Roland Scheidegger]

Am 07.11.2017 um 16:12 schrieb Brian Paul:
> Use the proper enum types for various variables.  Makes life in gdb
> a little nicer.  Note that the size of enum bitfields must be one
> larger so the high bit is always zero (for MSVC).
> 
> v2: also increase size of image_format bitfield, per Eric Engestrom.
> 
> Reviewed-by: Charmaine Lee <charmainel at vmware.com>
> ---
>  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..3e51936 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;
>     glsl_base_type tex_type:5;
>     unsigned tex_shadow:1;
> -   unsigned image_format:9;
> +   enum pipe_format image_format:10;

Due to this being a enum which can easily grow, this is of course quite
dangerous (doubly so now due to needing one bit more than you'd assume).
I think it would be nice if assignment somewhere were guarded by a
ASSERT(x.image_format == image_format) afterwards at least.
(Unfortunately I don't see a way to easily make a nice static assertion,
you'd have to use something like
STATIC_ASSERT(PIPE_FORMAT_COUNT < 1 << (10-1)) which isn't tied directly
to the bitfield definition.)

In any case,
Reviewed-by: Roland Scheidegger <sroland at vmware.com>


>     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
>