[Mesa-dev] [PATCH] st/mesa: use enum types instead of int/unsigned (v2)
Roland Scheidegger
sroland at vmware.com
Tue Nov 7 20:03:46 UTC 2017
Am 07.11.2017 um 20:52 schrieb Brian Paul:
> On 11/07/2017 11:09 AM, Roland Scheidegger wrote:
>> Am 07.11.2017 um 18:57 schrieb Brian Paul:
>>> 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 <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.)
>>>
>>> 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.
>> Yes, it's just a pity you can't do it with compile time asserts.
>> (Albeit I'd think the compiler will actually optimize it out in any case
>> if you've got any optimizations enabled, as it can still determine the
>> assertion will never fail at compile time.)
>
> GCC actually gives a compile-time warning when it can determine that the
> value won't fit in the field. That's almost as good as a static
> assertion. And the assertion seems to work at -O3 too.
Yes, that's what I meant - if it can figure out at compile time the
value won't fit and warn about it, it will also throw it out if it can
determine at compile time when it will fit, hence it nearly amounts to
the same as a static assert.
Roland
>
> -Brian
>
>
>>
>>>
>>> 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.
>> Yes, I think that should suffice and be a good solution.
>>
>> Roland
>>
>>
>>>
>>>
>>> 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 <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
>>>>>
>>>>
>>>
>>
>
More information about the mesa-dev
mailing list