[Mesa-dev] [PATCH 1/2] mesa: Add GL/GLSL plumbing for ARB_fragment_shader_interlock.

[Timothy Arceri]

On 19/04/17 11:12, Manolova, Plamena wrote:
> Thank you for reviewing Timothy!
>
> On Mon, Apr 17, 2017 at 7:10 PM, Timothy Arceri <tarceri at itsqueeze.com
> <mailto:tarceri at itsqueeze.com>> wrote:
>
>
>
>     On 18/04/17 11: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.manolova at intel.com
>         <mailto:plamena.manolova at 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(-)
>
>         diff --git a/src/compiler/glsl/ast.h b/src/compiler/glsl/ast.h
>         index 455cb81..f2f7e9e 100644
>         --- a/src/compiler/glsl/ast.h
>         +++ b/src/compiler/glsl/ast.h
>         @@ -617,6 +617,16 @@ struct ast_type_qualifier {
>                    * Flag set if GL_ARB_post_depth_coverage layout
>         qualifier is used.
>                    */
>                   unsigned post_depth_coverage:1;
>         +
>         +         /**
>         +          * Flags for the layout qualifers added by
>         ARB_fragment_shader_interlock
>         +          */
>         +
>         +         unsigned pixel_interlock_ordered:1;
>         +         unsigned pixel_interlock_unordered:1;
>         +         unsigned sample_interlock_ordered:1;
>         +         unsigned sample_interlock_unordered:1;
>
>
>     Samuel spent a bunch of time freeing up 4 bits of this flag to be
>     able to implement ARB_bindless_texture. This change will use up all
>     those free bits.
>
>     These only apply to the default in right? I wonder if it's possible
>     to split those qualifiers off from the layout qualifiers applied to
>     varyings?
>
>
> I think it should be possible to split them out, I can have a go at that.


I expect it will be tricky, but worth it if we can. Good luck. :)


>
>         +
>                   /**
>                    * Flag set if GL_INTEL_conservartive_rasterization
>         layout qualifier
>                    * is used.
>         diff --git a/src/compiler/glsl/ast_to_hir.cpp
>         b/src/compiler/glsl/ast_to_hir.cpp
>         index 9ea37f4..71c52ad 100644
>         --- a/src/compiler/glsl/ast_to_hir.cpp
>         +++ b/src/compiler/glsl/ast_to_hir.cpp
>         @@ -3717,6 +3717,27 @@ apply_layout_qualifier_to_variable(const
>         struct ast_type_qualifier *qual,
>                _mesa_glsl_error(loc, state, "post_depth_coverage layout
>         qualifier only "
>                                 "valid in fragment shader input layout
>         declaration.");
>             }
>         +
>         +   if (qual->flags.q.pixel_interlock_ordered) {
>         +      _mesa_glsl_error(loc, state, "pixel_interlock_ordered
>         layout qualifier only "
>         +                       "valid in fragment shader input layout
>         declaration.");
>         +   }
>         +
>         +   if (qual->flags.q.pixel_interlock_unordered) {
>         +      _mesa_glsl_error(loc, state, "pixel_interlock_unordered
>         layout qualifier only "
>         +                       "valid in fragment shader input layout
>         declaration.");
>         +   }
>         +
>         +
>         +   if (qual->flags.q.pixel_interlock_ordered) {
>         +      _mesa_glsl_error(loc, state, "sample_interlock_ordered
>         layout qualifier only "
>         +                       "valid in fragment shader input layout
>         declaration.");
>         +   }
>         +
>         +   if (qual->flags.q.pixel_interlock_unordered) {
>         +      _mesa_glsl_error(loc, state, "sample_interlock_unordered
>         layout qualifier only "
>         +                       "valid in fragment shader input layout
>         declaration.");
>         +   }
>
>     Here and below we duplicated the validation done in glsl_parser.yy.
>     Does the parser validation not catch everything?
>
>     Also there seems to be no link time validation to check that "only
>     one interlock mode can be used at any time" when we are compiling a
>     program that contains multiple fragment shaders.
>
>
>     Do you have any piglit tests to go with this series?
>
>
> Yes there's a test here: https://patchwork.freedesktop.org/patch/151133
>  I'll look into whether we actually need validation in both
> glsl_parser.yy and ast_type.cpp

That is just an execution test. Please also provide compile/link tests 
the exercise both valid and invalid shaders. With those you will easily 
be able to tell if the additional validation is required or not.

Thanks,
Tim