[Mesa-dev] [PATCH 7/7] mesa: add KHR_no_error support for glUseProgram
Timothy Arceri
tarceri at itsqueeze.com
Sun May 7 21:48:56 UTC 2017
On 08/05/17 06:29, Nicolai Hähnle wrote:
> On 04.05.2017 16:28, Nicolai Hähnle wrote:
>> On 04.05.2017 05:34, Timothy Arceri wrote:
>>>
>>>
>>> On 04/05/17 13:31, Dave Airlie wrote:
>>>>> +/* The ARB_separate_shader_object spec says:
>>>>> + *
>>>>> + * "The executable code for an individual shader stage is taken
>>>>> from
>>>>> + * the current program for that stage. If there is a current
>>>>> program
>>>>> + * object established by UseProgram, that program is considered
>>>>> current
>>>>> + * for all stages. Otherwise, if there is a bound program
>>>>> pipeline
>>>>> + * object (section 2.14.PPO), the program bound to the
>>>>> appropriate
>>>>> + * stage of the pipeline object is considered current."
>>>>> + */
>>>>> +#define
>>>>> USE_PROGRAM(no_error) \
>>>>> + if (program)
>>>>> { \
>>>>> + /* Attach shader state to the binding point
>>>>> */ \
>>>>> + _mesa_reference_pipeline_object(ctx, &ctx->_Shader,
>>>>> &ctx->Shader); \
>>>>> + /* Update the program
>>>>> */ \
>>>>> + _mesa_use_shader_program(ctx,
>>>>> shProg); \
>>>>> + } else
>>>>> { \
>>>>> + /* Must be done first: detach the progam
>>>>> */ \
>>>>> + _mesa_use_shader_program(ctx,
>>>>> shProg); \
>>>>> + /* Unattach shader_state binding point
>>>>> */ \
>>>>> + _mesa_reference_pipeline_object(ctx,
>>>>> &ctx->_Shader, \
>>>>> +
>>>>> ctx->Pipeline.Default); \
>>>>> + /* If a pipeline was bound, rebind it
>>>>> */ \
>>>>> + if (ctx->Pipeline.Current)
>>>>> { \
>>>>> +
>>>>> _mesa_BindProgramPipeline##no_error(ctx->Pipeline.Current->Name); \
>>>>> +
>>>>> }
>>>>> \
>>>>> +
>>>>> }
>>>>> \
>>>>> +
>>>>
>>>> why the macro, inline functions are a thing, or just one common
>>>> function that both entrypoints call.
>>>>
>>>
>>> So that we can avoid adding an if to call
>>>
>>> _mesa_BindProgramPipeline_no_error vs _mesa_BindProgramPipeline
>>
>> I have to say I also don't like this very much. Do you have benchmarks
>> to back up the usefulness of this?
>>
>> This is where C++ templates would be great, because there could just be
>> a boolean no_error template parameter. Maybe there's a way to get
>> somewhere similar in C, even if it requires some compiler extensions?
>
> I thought about this some more, and I feel that at least with gcc/clang,
> the always_inline attributes provides a reasonably elegant solution. I
> mean this code pattern:
>
> __attribute((always_inline))
> static inline void useprogram(GLuint program, bool check_error)
> {
> GET_CURRENT_CTX(ctx);
>
> ...
> }
>
> void GLAPIENTRY
> _mesa_UseProgram(GLuint program)
> {
> useprogram(program, true);
> }
>
> void GLAPIENTRY
> _mesa_UseProgram_no_error(GLuint program)
> {
> useprogram(program, false);
> }
>
> This would avoid the code duplication and macro magic that I'm finding a
> bit worrisome while still getting fully optimized code in both paths.
> This could also be used to clean up some of the other _no_error variants. >
> There's a bit of a question of other compiler support. At least judging
> from the online docs, MSVC's __forceinline should have the same effect,
> so it looks like all the relevant bases are covered.
>
> What do you think?
I'm happy with this solution, thanks. I'll send out a new version for
review.
>
> Cheers,
> Nicolai
>
>
>
>
>>
>> Cheers,
>> Nicolai
>>
>>
>>>
>>>> Dave.
>>>>
>>>>> +void GLAPIENTRY
>>>>> +_mesa_UseProgram_no_error(GLuint program)
>>>>> +{
>>>>> + GET_CURRENT_CONTEXT(ctx);
>>>>> + struct gl_shader_program *shProg = NULL;
>>>>> +
>>>>> + if (program) {
>>>>> + shProg = _mesa_lookup_shader_program(ctx, program);
>>>>> + }
>>>>> +
>>>>> + USE_PROGRAM(_no_error)
>>>>> +}
>>>>> +
>>>>>
>>>>> void GLAPIENTRY
>>>>> _mesa_UseProgram(GLuint program)
>>>>> {
>>>>> GET_CURRENT_CONTEXT(ctx);
>>>>> struct gl_shader_program *shProg = NULL;
>>>>>
>>>>> if (MESA_VERBOSE & VERBOSE_API)
>>>>> _mesa_debug(ctx, "glUseProgram %u\n", program);
>>>>>
>>>>> @@ -1875,44 +1915,21 @@ _mesa_UseProgram(GLuint program)
>>>>> return;
>>>>> }
>>>>>
>>>>> #ifdef DEBUG
>>>>> if (ctx->_Shader->Flags & GLSL_USE_PROG) {
>>>>> print_shader_info(shProg);
>>>>> }
>>>>> #endif
>>>>> }
>>>>>
>>>>> - /* The ARB_separate_shader_object spec says:
>>>>> - *
>>>>> - * "The executable code for an individual shader stage is
>>>>> taken from
>>>>> - * the current program for that stage. If there is a current
>>>>> program
>>>>> - * object established by UseProgram, that program is
>>>>> considered current
>>>>> - * for all stages. Otherwise, if there is a bound program
>>>>> pipeline
>>>>> - * object (section 2.14.PPO), the program bound to the
>>>>> appropriate
>>>>> - * stage of the pipeline object is considered current."
>>>>> - */
>>>>> - if (program) {
>>>>> - /* Attach shader state to the binding point */
>>>>> - _mesa_reference_pipeline_object(ctx, &ctx->_Shader,
>>>>> &ctx->Shader);
>>>>> - /* Update the program */
>>>>> - _mesa_use_shader_program(ctx, shProg);
>>>>> - } else {
>>>>> - /* Must be done first: detach the progam */
>>>>> - _mesa_use_shader_program(ctx, shProg);
>>>>> - /* Unattach shader_state binding point */
>>>>> - _mesa_reference_pipeline_object(ctx, &ctx->_Shader,
>>>>> ctx->Pipeline.Default);
>>>>> - /* If a pipeline was bound, rebind it */
>>>>> - if (ctx->Pipeline.Current) {
>>>>> - _mesa_BindProgramPipeline(ctx->Pipeline.Current->Name);
>>>>> - }
>>>>> - }
>>>>> + USE_PROGRAM()
>>>>> }
>>>>>
>>>>>
>>>>> void GLAPIENTRY
>>>>> _mesa_ValidateProgram(GLuint program)
>>>>> {
>>>>> GET_CURRENT_CONTEXT(ctx);
>>>>> validate_program(ctx, program);
>>>>> }
>>>>>
>>>>> diff --git a/src/mesa/main/shaderapi.h b/src/mesa/main/shaderapi.h
>>>>> index 99b4fe8..0a28185 100644
>>>>> --- a/src/mesa/main/shaderapi.h
>>>>> +++ b/src/mesa/main/shaderapi.h
>>>>> @@ -120,20 +120,22 @@ _mesa_IsProgram(GLuint name);
>>>>>
>>>>> extern GLboolean GLAPIENTRY
>>>>> _mesa_IsShader(GLuint name);
>>>>>
>>>>> extern void GLAPIENTRY
>>>>> _mesa_LinkProgram(GLuint programObj);
>>>>>
>>>>> extern void GLAPIENTRY
>>>>> _mesa_ShaderSource(GLuint, GLsizei, const GLchar* const *, const
>>>>> GLint *);
>>>>>
>>>>> +void GLAPIENTRY
>>>>> +_mesa_UseProgram_no_error(GLuint);
>>>>> extern void GLAPIENTRY
>>>>> _mesa_UseProgram(GLuint);
>>>>>
>>>>> extern void GLAPIENTRY
>>>>> _mesa_ValidateProgram(GLuint);
>>>>>
>>>>>
>>>>> extern void GLAPIENTRY
>>>>> _mesa_BindAttribLocation(GLuint program, GLuint, const GLchar *);
>>>>>
>>>>> --
>>>>> 2.9.3
>>>>>
>>>>> _______________________________________________
>>>>> mesa-dev mailing list
>>>>> mesa-dev at lists.freedesktop.org
>>>>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>>> _______________________________________________
>>> mesa-dev mailing list
>>> mesa-dev at lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>>
>>
>
>
More information about the mesa-dev
mailing list