On 11/07/2017 09:07 AM, Roland Scheidegger wrote:
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 <[email protected]>
---
  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.)

OK, I've come up with a simple runtime assertion to check for sufficient bitfield size:

/* Check that STRUCT::BITFIELD can hold MAXVAL */
#define ASSERT_BITFIELD_SIZE(STRUCT, FIELD, MAXVAL) \
   { \
      STRUCT s; \
      s.FIELD = MAXVAL; \
      assert((int) s.FIELD == MAXVAL && "Insufficient bitfield size!");  \
   }

This works identically for signed and unsigned enum fields with gcc and MSVC.

For enum bitfields, MSVC asserts if the extra padding bit is missing. But I haven't managed to make that happen with gcc.

I guess that's OK though. If I institute this assertion macro we won't detect the MSVC padding issue with gcc but I'll probably hit it soon enough on Windows if it happens.


FWIW, I also came up with a macro that can compute the number of bits in a bitfield:

#define SIZEOF_BITFIELD(STRUCT, FIELD, SIZE_OUT) \
{ \
   SIZE_OUT = 32; \
   unsigned i; \
   for (i = 0; i < 32; i++) { \
      struct STRUCT test; \
      test.FIELD = 1 << i; \
      if (abs(test.FIELD) != 1 << i) { \
         SIZE_OUT = i; \
         break; \
      } \
   } \
}

It handles int, unsigned and enum bitfields correctly with gcc and MSVC. I don't think we need it right now. But there it is if we need it someday.

I'm going to rework my patches to use the assertion macro.

-Brian



In any case,
Reviewed-by: Roland Scheidegger <[email protected]>


     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



_______________________________________________
mesa-dev mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Reply via email to