[Mesa-dev] [PATCH 2/3] glsl: Lower UBO and SSBO access in glsl linker

[Kristian Høgsberg]

On Thu, Nov 5, 2015 at 5:39 PM, Timothy Arceri
<timothy.arceri at collabora.com> wrote:
> On Wed, 2015-11-04 at 15:33 -0800, Kristian Høgsberg Kristensen wrote:
>> All GLSL IR consumers run this lowering pass so we can move it to the
>> linker. This moves the pass up quite a bit, but that's the point: it
>> needs to run before we throw away information about per-component vector
>> access.
>>
>> Signed-off-by: Kristian Høgsberg Kristensen <krh at bitplanet.net>
>> ---
>>  src/glsl/linker.cpp                        | 8 ++++++++
>>  src/mesa/drivers/dri/i965/brw_link.cpp     | 2 --
>>  src/mesa/drivers/dri/i965/brw_shader.cpp   | 2 ++
>>  src/mesa/main/mtypes.h                     | 2 ++
>>  src/mesa/state_tracker/st_extensions.c     | 1 +
>>  src/mesa/state_tracker/st_glsl_to_tgsi.cpp | 1 -
>>  6 files changed, 13 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/glsl/linker.cpp b/src/glsl/linker.cpp
>> index c35d87a..ea6a3f3 100644
>> --- a/src/glsl/linker.cpp
>> +++ b/src/glsl/linker.cpp
>> @@ -4449,6 +4449,14 @@ link_shaders(struct gl_context *ctx, struct
>> gl_shader_program *prog)
>>
>>     /* FINISHME: Assign fragment shader output locations. */
>>
>> +   for (unsigned i = 0; i < MESA_SHADER_STAGES; i++) {
>> +      if (prog->_LinkedShaders[i] == NULL)
>> +      continue;
>> +
>> +      if (ctx->Const.ShaderCompilerOptions[i].LowerBufferInterfaceBlocks)
>> +         lower_ubo_reference(prog->_LinkedShaders[i]);
>> +   }
>> +
>>  done:
>>     for (unsigned i = 0; i < MESA_SHADER_STAGES; i++) {
>>        free(shader_list[i]);
>> diff --git a/src/mesa/drivers/dri/i965/brw_link.cpp
>> b/src/mesa/drivers/dri/i965/brw_link.cpp
>> index f1e3860..2991173 100644
>> --- a/src/mesa/drivers/dri/i965/brw_link.cpp
>> +++ b/src/mesa/drivers/dri/i965/brw_link.cpp
>> @@ -157,8 +157,6 @@ process_glsl_ir(gl_shader_stage stage,
>>                   _mesa_shader_stage_to_abbrev(shader->Stage));
>>     }
>>
>> -   lower_ubo_reference(shader);
>> -
>>     bool progress;
>>     do {
>>        progress = false;
>> diff --git a/src/mesa/drivers/dri/i965/brw_shader.cpp
>> b/src/mesa/drivers/dri/i965/brw_shader.cpp
>> index 4ea297a..5adc986 100644
>> --- a/src/mesa/drivers/dri/i965/brw_shader.cpp
>> +++ b/src/mesa/drivers/dri/i965/brw_shader.cpp
>> @@ -148,6 +148,8 @@ brw_compiler_create(void *mem_ctx, const struct
>> brw_device_info *devinfo)
>>           compiler->glsl_compiler_options[i].EmitNoIndirectSampler = true;
>>
>>        compiler->glsl_compiler_options[i].NirOptions = nir_options;
>> +
>> +      compiler->glsl_compiler_options[i].LowerBufferInterfaceBlocks = true;
>>     }
>>
>>     return compiler;
>> diff --git a/src/mesa/main/mtypes.h b/src/mesa/main/mtypes.h
>> index d6c1eb8..800ad81 100644
>> --- a/src/mesa/main/mtypes.h
>> +++ b/src/mesa/main/mtypes.h
>> @@ -2874,6 +2874,8 @@ struct gl_shader_compiler_options
>>      */
>>     GLboolean OptimizeForAOS;
>>
>> +   GLboolean LowerBufferInterfaceBlocks; /**< Lower UBO and SSBO access to
>> intrinsics. */
>> +
>
> Does it even matter if this pass was to run for drivers that don't support
> UBO/SSBO?
>
> I assume all other drivers would want to run this pass, in which case is it
> really helpful to a this flag?

I think it was made optional in case some driver did not want UBO
access lowered to intrinsics, or wanted to run a custom lowering pass.
For example, if a driver wanted to push some UBO contents instead of
pulling, it could make that decision there. I don't know.

There are a lot of similar options that are also always enabled and I
didn't want to change whether or not GLSL IR consumers can decide to
run the lowering pass with this series.

Kristian

>>     const struct nir_shader_compiler_options *NirOptions;
>>  };
>>
>> diff --git a/src/mesa/state_tracker/st_extensions.c
>> b/src/mesa/state_tracker/st_extensions.c
>> index bd7cbcc..bbb9027 100644
>> --- a/src/mesa/state_tracker/st_extensions.c
>> +++ b/src/mesa/state_tracker/st_extensions.c
>> @@ -254,6 +254,7 @@ void st_init_limits(struct pipe_screen *screen,
>>
>>  PIPE_SHADER_CAP_MAX_UNROLL_ITERATIONS_HINT);
>>
>>        options->LowerClipDistance = true;
>> +      options->LowerBufferInterfaceBlocks = true;
>>     }
>>
>>     c->LowerTessLevel = true;
>> diff --git a/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
>> b/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
>> index ca00930..9ee6f8f 100644
>> --- a/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
>> +++ b/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
>> @@ -5822,7 +5822,6 @@ st_link_shader(struct gl_context *ctx, struct
>> gl_shader_program *prog)
>>                           (!ctx->Const.NativeIntegers ? INT_DIV_TO_MUL_RCP :
>> 0) |
>>                           (options->EmitNoSat ? SAT_TO_CLAMP : 0));
>>
>> -      lower_ubo_reference(prog->_LinkedShaders[i]);
>>        do_vec_index_to_cond_assign(ir);
>>        lower_vector_insert(ir, true);
>>        lower_quadop_vector(ir, false);