On 22/02/14 00:34, Ian Romanick wrote:
> On 02/21/2014 03:15 PM, Marek Olšák wrote:
>> The main reason _min exists is that it evaluates each parameter once,
>> while MIN2 evaluates each parameter twice, so in this case, MIN2 calls
>> the get_param function twice, which may not be desirable. The same for
>> the other macros.
>
> There is a GCC extension that cope with this, but I don't know if other
> compilers support it. It has existed in GCC for a decade or more.
> Directly from the GCC documentation:
>
> #define max(a,b) \
> ({ typeof (a) _a = (a); \
> typeof (b) _b = (b); \
> _a > _b ? _a : _b; })
>
> If some variation of this works with MSVC, maybe we could just "fix" our
> MIN2 and MAX2 macros.
>
> Perhaps Vinson could research our options here...
>
> If nothing else, this is the Nth time I've seen patches like this
> rejected. Perhaps the _min and _maxf functions could get a comment
> explaining why they exist?
>
As Ian said, if there is a reason for those to exist a single line of
comment would be greatly beneficial. IMHO all of those could be "flexed"
into some common(read mesa + gallium) headers, that handle the gcc
extension or fall-back to something suitable in case it's missing.
There are a bit too many cases in mesa that have duplication so pardon
if I'm not picking the correct one during my clean-up :)
-Emil
>> Marek
>>
>> On Fri, Feb 21, 2014 at 11:59 PM, Emil Velikov
>> <emil.l.veli...@gmail.com> wrote:
>>> Signed-off-by: Emil Velikov <emil.l.veli...@gmail.com>
>>> ---
>>> src/mesa/state_tracker/st_extensions.c | 27
>>> +++++++++++----------------
>>> 1 file changed, 11 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/src/mesa/state_tracker/st_extensions.c
>>> b/src/mesa/state_tracker/st_extensions.c
>>> index e43e7b4..a909b71 100644
>>> --- a/src/mesa/state_tracker/st_extensions.c
>>> +++ b/src/mesa/state_tracker/st_extensions.c
>>> @@ -39,11 +39,6 @@
>>> #include "st_extensions.h"
>>> #include "st_format.h"
>>>
>>> -static unsigned _min(unsigned a, unsigned b)
>>> -{
>>> - return (a < b) ? a : b;
>>> -}
>>> -
>>> static float _maxf(float a, float b)
>>> {
>>> return (a > b) ? a : b;
>>> @@ -72,19 +67,19 @@ void st_init_limits(struct st_context *st)
>>> boolean can_ubo = TRUE;
>>>
>>> c->MaxTextureLevels
>>> - = _min(screen->get_param(screen, PIPE_CAP_MAX_TEXTURE_2D_LEVELS),
>>> + = MIN2(screen->get_param(screen, PIPE_CAP_MAX_TEXTURE_2D_LEVELS),
>>> MAX_TEXTURE_LEVELS);
>>>
>>> c->Max3DTextureLevels
>>> - = _min(screen->get_param(screen, PIPE_CAP_MAX_TEXTURE_3D_LEVELS),
>>> + = MIN2(screen->get_param(screen, PIPE_CAP_MAX_TEXTURE_3D_LEVELS),
>>> MAX_3D_TEXTURE_LEVELS);
>>>
>>> c->MaxCubeTextureLevels
>>> - = _min(screen->get_param(screen,
>>> PIPE_CAP_MAX_TEXTURE_CUBE_LEVELS),
>>> + = MIN2(screen->get_param(screen,
>>> PIPE_CAP_MAX_TEXTURE_CUBE_LEVELS),
>>> MAX_CUBE_TEXTURE_LEVELS);
>>>
>>> c->MaxTextureRectSize
>>> - = _min(1 << (c->MaxTextureLevels - 1), MAX_TEXTURE_RECT_SIZE);
>>> + = MIN2(1 << (c->MaxTextureLevels - 1), MAX_TEXTURE_RECT_SIZE);
>>>
>>> c->MaxArrayTextureLayers
>>> = screen->get_param(screen, PIPE_CAP_MAX_TEXTURE_ARRAY_LAYERS);
>>> @@ -168,7 +163,7 @@ void st_init_limits(struct st_context *st)
>>> }
>>>
>>> pc->MaxTextureImageUnits =
>>> - _min(screen->get_shader_param(screen, sh,
>>> + MIN2(screen->get_shader_param(screen, sh,
>>>
>>> PIPE_SHADER_CAP_MAX_TEXTURE_SAMPLERS),
>>> MAX_TEXTURE_IMAGE_UNITS);
>>>
>>> @@ -185,7 +180,7 @@ void st_init_limits(struct st_context *st)
>>> pc->MaxTemps = pc->MaxNativeTemps =
>>> screen->get_shader_param(screen, sh,
>>> PIPE_SHADER_CAP_MAX_TEMPS);
>>> pc->MaxAddressRegs = pc->MaxNativeAddressRegs =
>>> - _min(screen->get_shader_param(screen, sh,
>>> PIPE_SHADER_CAP_MAX_ADDRS),
>>> + MIN2(screen->get_shader_param(screen, sh,
>>> PIPE_SHADER_CAP_MAX_ADDRS),
>>> MAX_PROGRAM_ADDRESS_REGS);
>>> pc->MaxParameters = pc->MaxNativeParameters =
>>> screen->get_shader_param(screen, sh,
>>> PIPE_SHADER_CAP_MAX_CONSTS);
>>> @@ -196,7 +191,7 @@ void st_init_limits(struct st_context *st)
>>> screen->get_shader_param(screen, sh,
>>> PIPE_SHADER_CAP_MAX_CONST_BUFFERS);
>>> if (pc->MaxUniformBlocks)
>>> pc->MaxUniformBlocks -= 1; /* The first one is for
>>> ordinary uniforms. */
>>> - pc->MaxUniformBlocks = _min(pc->MaxUniformBlocks,
>>> MAX_UNIFORM_BUFFERS);
>>> + pc->MaxUniformBlocks = MIN2(pc->MaxUniformBlocks,
>>> MAX_UNIFORM_BUFFERS);
>>>
>>> pc->MaxCombinedUniformComponents = (pc->MaxUniformComponents +
>>> c->MaxUniformBlockSize / 4 *
>>> @@ -240,16 +235,16 @@ void st_init_limits(struct st_context *st)
>>> }
>>>
>>> c->MaxCombinedTextureImageUnits =
>>> - _min(c->Program[MESA_SHADER_VERTEX].MaxTextureImageUnits +
>>> + MIN2(c->Program[MESA_SHADER_VERTEX].MaxTextureImageUnits +
>>> c->Program[MESA_SHADER_GEOMETRY].MaxTextureImageUnits +
>>> c->Program[MESA_SHADER_FRAGMENT].MaxTextureImageUnits,
>>> MAX_COMBINED_TEXTURE_IMAGE_UNITS);
>>>
>>> /* This depends on program constants. */
>>> c->MaxTextureCoordUnits
>>> - = _min(c->Program[MESA_SHADER_FRAGMENT].MaxTextureImageUnits,
>>> MAX_TEXTURE_COORD_UNITS);
>>> + = MIN2(c->Program[MESA_SHADER_FRAGMENT].MaxTextureImageUnits,
>>> MAX_TEXTURE_COORD_UNITS);
>>>
>>> - c->MaxTextureUnits =
>>> _min(c->Program[MESA_SHADER_FRAGMENT].MaxTextureImageUnits,
>>> c->MaxTextureCoordUnits);
>>> + c->MaxTextureUnits =
>>> MIN2(c->Program[MESA_SHADER_FRAGMENT].MaxTextureImageUnits,
>>> c->MaxTextureCoordUnits);
>>>
>>> c->Program[MESA_SHADER_VERTEX].MaxAttribs =
>>> MIN2(c->Program[MESA_SHADER_VERTEX].MaxAttribs, 16);
>>>
>>> @@ -749,7 +744,7 @@ void st_init_extensions(struct st_context *st)
>>> ctx->Extensions.ARB_texture_buffer_object = GL_TRUE;
>>>
>>> ctx->Const.MaxTextureBufferSize =
>>> - _min(screen->get_param(screen,
>>> PIPE_CAP_MAX_TEXTURE_BUFFER_SIZE),
>>> + MIN2(screen->get_param(screen,
>>> PIPE_CAP_MAX_TEXTURE_BUFFER_SIZE),
>>> (1u << 31) - 1);
>>> ctx->Const.TextureBufferOffsetAlignment =
>>> screen->get_param(screen,
>>> PIPE_CAP_TEXTURE_BUFFER_OFFSET_ALIGNMENT);
>>> --
>>> 1.9.0
>>>
>>> _______________________________________________
>>> mesa-dev mailing list
>>> mesa-dev@lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>>
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
