[Mesa-dev] [PATCH] st/mesa: use enum types instead of int/unsigned (v2)
Brian Paul
brianp at vmware.com
Tue Nov 7 19:52:52 UTC 2017
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.
-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