Thank you for reviewing Nicolai!
On Tue, Apr 18, 2017 at 4:17 AM, Nicolai Hähnle <nhaeh...@gmail.com> wrote:
> On 18.04.2017 03:25, Plamena Manolova wrote:
>
>> This extension provides new GLSL built-in functions
>> beginInvocationInterlockARB() and endInvocationInterlockARB()
>> that delimit a critical section of fragment shader code. For
>> pairs of shader invocations with "overlapping" coverage in a
>> given pixel, the OpenGL implementation will guarantee that the
>> critical section of the fragment shader will be executed for
>> only one fragment at a time.
>>
>> Signed-off-by: Plamena Manolova <plamena.manol...@intel.com>
>> ---
>> src/compiler/glsl/ast.h | 10 +++
>> src/compiler/glsl/ast_to_hir.cpp | 21 ++++++
>> src/compiler/glsl/ast_type.cpp | 90 ++++++++++++++++++++++++-
>> src/compiler/glsl/builtin_functions.cpp | 79 ++++++++++++++++++++++
>> src/compiler/glsl/glsl_parser.yy | 109
>> +++++++++++++++++++++++++++++++
>> src/compiler/glsl/glsl_parser_extras.cpp | 13 ++++
>> src/compiler/glsl/glsl_parser_extras.h | 7 ++
>> src/compiler/glsl/glsl_to_nir.cpp | 12 ++++
>> src/compiler/glsl/ir.h | 2 +
>> src/compiler/glsl/linker.cpp | 8 +++
>> src/compiler/nir/nir_intrinsics.h | 2 +
>> src/compiler/shader_info.h | 5 ++
>> src/mesa/main/extensions_table.h | 1 +
>> src/mesa/main/mtypes.h | 5 ++
>> 14 files changed, 363 insertions(+), 1 deletion(-)
>>
>> [snip]
>
> @@ -651,6 +655,86 @@ ast_type_qualifier::merge_into_in_qualifier(YYLTYPE
>> *loc,
>> r = false;
>> }
>>
>> + if (state->in_qualifier->flags.q.pixel_interlock_ordered) {
>> + if (state->in_qualifier->flags.q.pixel_interlock_unordered ||
>> + state->in_qualifier->flags.q.sample_interlock_ordered ||
>> + state->in_qualifier->flags.q.sample_interlock_unordered) {
>> + _mesa_glsl_error(loc, state, "only one interlock mode can be
>> used at "
>> + "any time.");
>> + r = false;
>> + } else {
>> + if (!state->ctx->Multisample.Enabled) {
>> + state->fs_pixel_interlock_ordered = true;
>> + state->in_qualifier->flags.q.pixel_interlock_ordered =
>> false;
>> + } else {
>> + _mesa_glsl_error(loc, state,
>> + "pixel_interlock_ordered can only be used
>> when "
>> + "multisampling is disabled.");
>> + r = false;
>> + }
>> + }
>> + }
>> +
>> + if (state->in_qualifier->flags.q.pixel_interlock_unordered) {
>> + if (state->in_qualifier->flags.q.pixel_interlock_ordered ||
>> + state->in_qualifier->flags.q.sample_interlock_ordered ||
>> + state->in_qualifier->flags.q.sample_interlock_unordered) {
>> + _mesa_glsl_error(loc, state, "only one interlock mode can be
>> used at "
>> + "any time.");
>> + r = false;
>> + } else {
>> + if (!state->ctx->Multisample.Enabled) {
>> + state->fs_pixel_interlock_unordered = true;
>> + state->in_qualifier->flags.q.pixel_interlock_unordered =
>> false;
>> + } else {
>> + _mesa_glsl_error(loc, state,
>> + "pixel_interlock_unordered can only be used
>> when "
>> + "multisampling is disabled.");
>> + r = false;
>> + }
>> + }
>> + }
>> +
>> + if (state->in_qualifier->flags.q.sample_interlock_ordered) {
>> + if (state->in_qualifier->flags.q.pixel_interlock_ordered ||
>> + state->in_qualifier->flags.q.pixel_interlock_unordered ||
>> + state->in_qualifier->flags.q.sample_interlock_unordered) {
>> + _mesa_glsl_error(loc, state, "only one interlock mode can be
>> used at "
>> + "any time.");
>> + r = false;
>> + } else {
>> + if (state->ctx->Multisample.Enabled) {
>> + state->fs_sample_interlock_ordered = true;
>> + state->in_qualifier->flags.q.sample_interlock_ordered =
>> false;
>> + } else {
>> + _mesa_glsl_error(loc, state,
>> + "sample_interlock_ordered can only be used
>> when "
>> + "multisampling is enabled.");
>> + r = false;
>> + }
>> + }
>> + }
>> +
>> + if (state->in_qualifier->flags.q.sample_interlock_unordered) {
>> + if (state->in_qualifier->flags.q.pixel_interlock_ordered ||
>> + state->in_qualifier->flags.q.pixel_interlock_unordered ||
>> + state->in_qualifier->flags.q.sample_interlock_ordered) {
>> + _mesa_glsl_error(loc, state, "only one interlock mode can be
>> used at "
>> + "any time.");
>> + r = false;
>> + } else {
>> + if (state->ctx->Multisample.Enabled) {
>> + state->fs_sample_interlock_unordered = true;
>> + state->in_qualifier->flags.q.sample_interlock_unordered =
>> false;
>> + } else {
>> + _mesa_glsl_error(loc, state,
>> + "sample_interlock_unordered can only be
>> used when "
>> + "multisampling is enabled.\n");
>> + r = false;
>> + }
>> + }
>> + }
>>
>
> There is a lot of code (almost-)duplication here.
>
> At the very least, the "only one interlock mode" check should be
> simplified by just checking
>
> if (state->in_qualifier->flags.q.sample_interlock_ordered +
> state->in_qualifier->flags.q.sample_interlock_unordered +
> state->in_qualifier->flags.q.pixel_interlock_ordered +
> state->in_qualifier->flags.q.pixel_interlock_unordered > 1)
> ...
>
> The remaining duplication could be reduced by temporarily defining a
> preprocessor macro. Personally, that's what I would do, but I'm okay with
> other people have different stylistic preferences.
>
>
[snip]
Those if-statements could definitely be simplified, I'll go with your
approach.
>
> diff --git a/src/compiler/glsl/glsl_parser.yy
>> b/src/compiler/glsl/glsl_parser.yy
>> index e703073..1e23fd2 100644
>> --- a/src/compiler/glsl/glsl_parser.yy
>> +++ b/src/compiler/glsl/glsl_parser.yy
>> @@ -1456,6 +1456,115 @@ layout_qualifier_id:
>> }
>> }
>>
>> + $$.flags.q.pixel_interlock_ordered = match_layout_qualifier($1,
>> + "pixel_interlock_ordered", state) == 0 ? 1 : 0;
>> + $$.flags.q.pixel_interlock_unordered = match_layout_qualifier($1,
>> + "pixel_interlock_unordered", state) == 0 ? 1 : 0;
>> + $$.flags.q.sample_interlock_ordered = match_layout_qualifier($1,
>> + "sample_interlock_ordered", state) == 0 ? 1 : 0;
>> + $$.flags.q.sample_interlock_unordered = match_layout_qualifier($1,
>> + "sample_interlock_unordered", state) == 0 ? 1 : 0;
>> +
>> + if ($$.flags.q.pixel_interlock_ordered == 1) {
>> + if (state->stage != MESA_SHADER_FRAGMENT) {
>> + _mesa_glsl_error(& @1, state,
>> + "pixel_interlock_ordered layout qualifier
>> only "
>> + "valid in fragment shaders.");
>> + $$.flags.q.pixel_interlock_ordered = 0;
>> + }
>> +
>> + if ($$.flags.q.pixel_interlock_unordered == 1 ||
>> + $$.flags.q.sample_interlock_ordered == 1 ||
>> + $$.flags.q.sample_interlock_unordered == 1) {
>> + _mesa_glsl_error(& @1, state,
>> + "only one interlock mode can be used at any
>> "
>> + "time.");
>> + $$.flags.q.pixel_interlock_ordered = 0;
>> + }
>> +
>> + if (state->ctx->Multisample.Enabled) {
>> + _mesa_glsl_error(& @1, state,
>> + "pixel_interlock_ordered can only be used
>> when "
>> + "multisampling is disabled.");
>> + $$.flags.q.pixel_interlock_ordered = 0;
>> + }
>> + }
>>
>
> Are these checks redundant with the checks above?
>
> Cheers,
> Nicolai
>
>
> +
>> + if ($$.flags.q.pixel_interlock_unordered == 1) {
>> + if (state->stage != MESA_SHADER_FRAGMENT) {
>> + _mesa_glsl_error(& @1, state,
>> + "pixel_interlock_unordered layout qualifier
>> only "
>> + "valid in fragment shaders.");
>> + $$.flags.q.pixel_interlock_unordered = 0;
>> + }
>> +
>> + if ($$.flags.q.pixel_interlock_ordered == 1 ||
>> + $$.flags.q.sample_interlock_ordered == 1 ||
>> + $$.flags.q.sample_interlock_unordered == 1) {
>> + _mesa_glsl_error(& @1, state,
>> + "only one interlock mode can be used at any
>> "
>> + "time.");
>> + $$.flags.q.pixel_interlock_unordered = 0;
>> + }
>> +
>> + if (state->ctx->Multisample.Enabled) {
>> + _mesa_glsl_error(& @1, state,
>> + "pixel_interlock_unordered can only be used
>> when "
>> + "multisampling is disabled.");
>> + $$.flags.q.pixel_interlock_unordered = 0;
>> + }
>> + }
>> +
>> + if ($$.flags.q.sample_interlock_ordered == 1) {
>> + if (state->stage != MESA_SHADER_FRAGMENT) {
>> + _mesa_glsl_error(& @1, state,
>> + "sample_interlock_ordered layout qualifier
>> only "
>> + "valid in fragment shaders.");
>> + $$.flags.q.sample_interlock_ordered = 0;
>> + }
>> +
>> + if ($$.flags.q.pixel_interlock_ordered == 1 ||
>> + $$.flags.q.pixel_interlock_unordered == 1 ||
>> + $$.flags.q.sample_interlock_unordered == 1) {
>> + _mesa_glsl_error(& @1, state,
>> + "only one interlock mode can be used at any
>> "
>> + "time.");
>> + $$.flags.q.sample_interlock_ordered = 0;
>> + }
>> +
>> + if (!state->ctx->Multisample.Enabled) {
>> + _mesa_glsl_error(& @1, state,
>> + "sample_interlock_ordered can only be used
>> when "
>> + "multisampling is enabled.");
>> + $$.flags.q.sample_interlock_ordered = 0;
>> + }
>> + }
>> +
>> + if ($$.flags.q.sample_interlock_unordered == 1) {
>> + if (state->stage != MESA_SHADER_FRAGMENT) {
>> + _mesa_glsl_error(& @1, state,
>> + "sample_interlock_unordered layout
>> qualifier only "
>> + "valid in fragment shaders.");
>> + $$.flags.q.sample_interlock_unordered = 0;
>> + }
>> +
>> + if ($$.flags.q.pixel_interlock_ordered == 1 ||
>> + $$.flags.q.pixel_interlock_unordered == 1 ||
>> + $$.flags.q.sample_interlock_ordered == 1) {
>> + _mesa_glsl_error(& @1, state,
>> + "only one interlock mode can be used at any
>> "
>> + "time.");
>> + $$.flags.q.sample_interlock_unordered = 0;
>> + }
>> +
>> + if (!state->ctx->Multisample.Enabled) {
>> + _mesa_glsl_error(& @1, state,
>> + "sample_interlock_unordered can only be
>> used when "
>> + "multisampling is enabled.");
>> + $$.flags.q.sample_interlock_unordered = 0;
>> + }
>> + }
>> +
>> /* Layout qualifiers for tessellation evaluation shaders. */
>> if (!$$.flags.i) {
>> static const struct {
>> diff --git a/src/compiler/glsl/glsl_parser_extras.cpp
>> b/src/compiler/glsl/glsl_parser_extras.cpp
>> index eb12eff..68ebd5c 100644
>> --- a/src/compiler/glsl/glsl_parser_extras.cpp
>> +++ b/src/compiler/glsl/glsl_parser_extras.cpp
>> @@ -300,6 +300,10 @@ _mesa_glsl_parse_state::_mesa_glsl_parse_state(struct
>> gl_context *_ctx,
>> this->fs_early_fragment_tests = false;
>> this->fs_inner_coverage = false;
>> this->fs_post_depth_coverage = false;
>> + this->fs_pixel_interlock_ordered = false;
>> + this->fs_pixel_interlock_unordered = false;
>> + this->fs_sample_interlock_ordered = false;
>> + this->fs_sample_interlock_unordered = false;
>> this->fs_blend_support = 0;
>> memset(this->atomic_counter_offsets, 0,
>> sizeof(this->atomic_counter_offsets));
>> @@ -619,6 +623,7 @@ static const _mesa_glsl_extension
>> _mesa_glsl_supported_extensions[] = {
>> EXT(ARB_explicit_uniform_location),
>> EXT(ARB_fragment_coord_conventions),
>> EXT(ARB_fragment_layer_viewport),
>> + EXT(ARB_fragment_shader_interlock),
>> EXT(ARB_gpu_shader5),
>> EXT(ARB_gpu_shader_fp64),
>> EXT(ARB_gpu_shader_int64),
>> @@ -1720,6 +1725,10 @@ set_shader_inout_layout(struct gl_shader *shader,
>> assert(!state->fs_early_fragment_tests);
>> assert(!state->fs_inner_coverage);
>> assert(!state->fs_post_depth_coverage);
>> + assert(!state->fs_pixel_interlock_ordered);
>> + assert(!state->fs_pixel_interlock_unordered);
>> + assert(!state->fs_sample_interlock_ordered);
>> + assert(!state->fs_sample_interlock_unordered);
>> }
>>
>> for (unsigned i = 0; i < MAX_FEEDBACK_BUFFERS; i++) {
>> @@ -1841,6 +1850,10 @@ set_shader_inout_layout(struct gl_shader *shader,
>> shader->EarlyFragmentTests = state->fs_early_fragment_tests;
>> shader->InnerCoverage = state->fs_inner_coverage;
>> shader->PostDepthCoverage = state->fs_post_depth_coverage;
>> + shader->PixelInterlockOrdered = state->fs_pixel_interlock_ordered;
>> + shader->PixelInterlockUnordered = state->fs_pixel_interlock_unor
>> dered;
>> + shader->SampleInterlockOrdered = state->fs_sample_interlock_ord
>> ered;
>> + shader->SampleInterlockUnordered = state->fs_sample_interlock_uno
>> rdered;
>> shader->BlendSupport = state->fs_blend_support;
>> break;
>>
>> diff --git a/src/compiler/glsl/glsl_parser_extras.h
>> b/src/compiler/glsl/glsl_parser_extras.h
>> index 6c3bc8a..56218cc 100644
>> --- a/src/compiler/glsl/glsl_parser_extras.h
>> +++ b/src/compiler/glsl/glsl_parser_extras.h
>> @@ -616,6 +616,8 @@ struct _mesa_glsl_parse_state {
>> bool ARB_fragment_coord_conventions_warn;
>> bool ARB_fragment_layer_viewport_enable;
>> bool ARB_fragment_layer_viewport_warn;
>> + bool ARB_fragment_shader_interlock_enable;
>> + bool ARB_fragment_shader_interlock_warn;
>> bool ARB_gpu_shader5_enable;
>> bool ARB_gpu_shader5_warn;
>> bool ARB_gpu_shader_fp64_enable;
>> @@ -810,6 +812,11 @@ struct _mesa_glsl_parse_state {
>>
>> bool fs_post_depth_coverage;
>>
>> + bool fs_pixel_interlock_ordered;
>> + bool fs_pixel_interlock_unordered;
>> + bool fs_sample_interlock_ordered;
>> + bool fs_sample_interlock_unordered;
>> +
>> unsigned fs_blend_support;
>>
>> /**
>> diff --git a/src/compiler/glsl/glsl_to_nir.cpp
>> b/src/compiler/glsl/glsl_to_nir.cpp
>> index 870d457..82b872c 100644
>> --- a/src/compiler/glsl/glsl_to_nir.cpp
>> +++ b/src/compiler/glsl/glsl_to_nir.cpp
>> @@ -734,6 +734,12 @@ nir_visitor::visit(ir_call *ir)
>> case ir_intrinsic_shader_clock:
>> op = nir_intrinsic_shader_clock;
>> break;
>> + case ir_intrinsic_begin_invocation_interlock_ARB:
>> + op = nir_intrinsic_begin_invocation_interlock_ARB;
>> + break;
>> + case ir_intrinsic_end_invocation_interlock_ARB:
>> + op = nir_intrinsic_end_invocation_interlock_ARB;
>> + break;
>> case ir_intrinsic_group_memory_barrier:
>> op = nir_intrinsic_group_memory_barrier;
>> break;
>> @@ -934,6 +940,12 @@ nir_visitor::visit(ir_call *ir)
>> instr->num_components = 2;
>> nir_builder_instr_insert(&b, &instr->instr);
>> break;
>> + case nir_intrinsic_begin_invocation_interlock_ARB:
>> + nir_builder_instr_insert(&b, &instr->instr);
>> + break;
>> + case nir_intrinsic_end_invocation_interlock_ARB:
>> + nir_builder_instr_insert(&b, &instr->instr);
>> + break;
>> case nir_intrinsic_store_ssbo: {
>> exec_node *param = ir->actual_parameters.get_head();
>> ir_rvalue *block = ((ir_instruction *)param)->as_rvalue();
>> diff --git a/src/compiler/glsl/ir.h b/src/compiler/glsl/ir.h
>> index d7a81c5..a404d00 100644
>> --- a/src/compiler/glsl/ir.h
>> +++ b/src/compiler/glsl/ir.h
>> @@ -1097,6 +1097,8 @@ enum ir_intrinsic_id {
>> ir_intrinsic_memory_barrier_buffer,
>> ir_intrinsic_memory_barrier_image,
>> ir_intrinsic_memory_barrier_shared,
>> + ir_intrinsic_begin_invocation_interlock_ARB,
>> + ir_intrinsic_end_invocation_interlock_ARB,
>>
>> ir_intrinsic_shared_load,
>> ir_intrinsic_shared_store = MAKE_INTRINSIC_FOR_TYPE(store, shared),
>> diff --git a/src/compiler/glsl/linker.cpp b/src/compiler/glsl/linker.cpp
>> index 7ace01d..f06eaf7 100644
>> --- a/src/compiler/glsl/linker.cpp
>> +++ b/src/compiler/glsl/linker.cpp
>> @@ -1895,6 +1895,14 @@ link_fs_inout_layout_qualifiers(struct
>> gl_shader_program *prog,
>> linked_shader->Program->info.fs.inner_coverage |=
>> shader->InnerCoverage;
>> linked_shader->Program->info.fs.post_depth_coverage |=
>> shader->PostDepthCoverage;
>> + linked_shader->Program->info.fs.pixel_interlock_ordered |=
>> + shader->PixelInterlockOrdered;
>> + linked_shader->Program->info.fs.pixel_interlock_unordered |=
>> + shader->PixelInterlockUnordered;
>> + linked_shader->Program->info.fs.sample_interlock_ordered |=
>> + shader->SampleInterlockOrdered;
>> + linked_shader->Program->info.fs.sample_interlock_unordered |=
>> + shader->SampleInterlockUnordered;
>>
>> linked_shader->Program->sh.fs.BlendSupport |=
>> shader->BlendSupport;
>> }
>> diff --git a/src/compiler/nir/nir_intrinsics.h
>> b/src/compiler/nir/nir_intrinsics.h
>> index 3a519a7..82dac3d 100644
>> --- a/src/compiler/nir/nir_intrinsics.h
>> +++ b/src/compiler/nir/nir_intrinsics.h
>> @@ -104,6 +104,8 @@ BARRIER(memory_barrier_buffer)
>> BARRIER(memory_barrier_image)
>> BARRIER(memory_barrier_shared)
>>
>> +BARRIER(begin_invocation_interlock_ARB)
>> +BARRIER(end_invocation_interlock_ARB)
>> /** A conditional discard, with a single boolean source. */
>> INTRINSIC(discard_if, 1, ARR(1), false, 0, 0, 0, xx, xx, xx, 0)
>>
>> diff --git a/src/compiler/shader_info.h b/src/compiler/shader_info.h
>> index a670841..3d7282f 100644
>> --- a/src/compiler/shader_info.h
>> +++ b/src/compiler/shader_info.h
>> @@ -127,6 +127,11 @@ typedef struct shader_info {
>>
>> bool post_depth_coverage;
>>
>> + bool pixel_interlock_ordered;
>> + bool pixel_interlock_unordered;
>> + bool sample_interlock_ordered;
>> + bool sample_interlock_unordered;
>> +
>> /** gl_FragDepth layout for ARB_conservative_depth. */
>> enum gl_frag_depth_layout depth_layout;
>> } fs;
>> diff --git a/src/mesa/main/extensions_table.h
>> b/src/mesa/main/extensions_table.h
>> index d11cb0f..e64a88a 100644
>> --- a/src/mesa/main/extensions_table.h
>> +++ b/src/mesa/main/extensions_table.h
>> @@ -67,6 +67,7 @@ EXT(ARB_fragment_layer_viewport ,
>> ARB_fragment_layer_viewport
>> EXT(ARB_fragment_program , ARB_fragment_program
>> , GLL, x , x , x , 2002)
>> EXT(ARB_fragment_program_shadow ,
>> ARB_fragment_program_shadow , GLL, x , x , x , 2003)
>> EXT(ARB_fragment_shader , ARB_fragment_shader
>> , GLL, GLC, x , x , 2002)
>> +EXT(ARB_fragment_shader_interlock ,
>> ARB_fragment_shader_interlock , GLL, GLC, x , x , 2015)
>> EXT(ARB_framebuffer_no_attachments , ARB_framebuffer_no_attachments
>> , GLL, GLC, x , x , 2012)
>> EXT(ARB_framebuffer_object , ARB_framebuffer_object
>> , GLL, GLC, x , x , 2005)
>> EXT(ARB_framebuffer_sRGB , EXT_framebuffer_sRGB
>> , GLL, GLC, x , x , 1998)
>> diff --git a/src/mesa/main/mtypes.h b/src/mesa/main/mtypes.h
>> index c4fab9d..608345d 100644
>> --- a/src/mesa/main/mtypes.h
>> +++ b/src/mesa/main/mtypes.h
>> @@ -2558,6 +2558,10 @@ struct gl_shader
>> bool uses_gl_fragcoord;
>>
>> bool PostDepthCoverage;
>> + bool PixelInterlockOrdered;
>> + bool PixelInterlockUnordered;
>> + bool SampleInterlockOrdered;
>> + bool SampleInterlockUnordered;
>> bool InnerCoverage;
>>
>> /**
>> @@ -3967,6 +3971,7 @@ struct gl_extensions
>> GLboolean ARB_fragment_shader;
>> GLboolean ARB_framebuffer_no_attachments;
>> GLboolean ARB_framebuffer_object;
>> + GLboolean ARB_fragment_shader_interlock;
>> GLboolean ARB_enhanced_layouts;
>> GLboolean ARB_explicit_attrib_location;
>> GLboolean ARB_explicit_uniform_location;
>> --
>> 2.9.3
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>>
>>
>
> --
> Lerne, wie die Welt wirklich ist,
> Aber vergiss niemals, wie sie sein sollte.
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
