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

2015-11-10 Thread Kristian Høgsberg 
On Tue, Nov 10, 2015 at 12:09 AM, Iago Toral  wrote:
> On Mon, 2015-11-09 at 16:52 +0100, Iago Toral 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 
>> > ---
>> >  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]);
>> > +   }
>> > +
>>
>> It probably makes more sense to rewrite this loop as:
>>
>> if (ctx->Const.ShaderCompilerOptions[i].LowerBufferInterfaceBlocks) {
>>for (unsigned i = 0; i < MESA_SHADER_STAGES; i++) {
>>   if (prog->_LinkedShaders[i] != NULL)
>>  lower_ubo_reference(prog->_LinkedShaders[i]);
>>}
>> }
>>
>> With that change, and assuming that this change is not responsible for
>> the shader-db regressions posted by Jason:
>
> Forget about that, I did not notice that LowerBufferInterfaceBlocks is
> set by stage. You can keep the Rb for the original version.
>
> Iago

Thanks Iago. The shader-db regression  was down to me confusing
ir_type_variable with ir_type_dereference_variable in the
src/glsl/opt_dead_code_local.cpp hunk.

Kristian

>> Reviewed-by: Iago Toral Quiroga 
>>
>> >  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. */
>> > +
>> > 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 
>> >

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

2015-11-10 Thread Iago Toral 
On Mon, 2015-11-09 at 16:52 +0100, Iago Toral 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 
> > ---
> >  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]);
> > +   }
> > +
> 
> It probably makes more sense to rewrite this loop as:
> 
> if (ctx->Const.ShaderCompilerOptions[i].LowerBufferInterfaceBlocks) {
>for (unsigned i = 0; i < MESA_SHADER_STAGES; i++) {
>   if (prog->_LinkedShaders[i] != NULL)
>  lower_ubo_reference(prog->_LinkedShaders[i]);
>}
> }
> 
> With that change, and assuming that this change is not responsible for
> the shader-db regressions posted by Jason:

Forget about that, I did not notice that LowerBufferInterfaceBlocks is
set by stage. You can keep the Rb for the original version.

Iago

> Reviewed-by: Iago Toral Quiroga 
> 
> >  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. */
> > +
> > 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);
> >

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

2015-11-09 Thread Iago Toral 
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 
> ---
>  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]);
> +   }
> +

It probably makes more sense to rewrite this loop as:

if (ctx->Const.ShaderCompilerOptions[i].LowerBufferInterfaceBlocks) {
   for (unsigned i = 0; i < MESA_SHADER_STAGES; i++) {
  if (prog->_LinkedShaders[i] != NULL)
 lower_ubo_reference(prog->_LinkedShaders[i]);
   }
}

With that change, and assuming that this change is not responsible for
the shader-db regressions posted by Jason:

Reviewed-by: Iago Toral Quiroga 

>  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. */
> +
> 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);


___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

2015-11-05 Thread Kristian Høgsberg 
On Thu, Nov 5, 2015 at 5:39 PM, Timothy Arceri
 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 
>> ---
>>  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);
___
mesa-dev mailing list

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

2015-11-05 Thread Timothy Arceri 
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 
> ---
>  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?

> 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);
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

2015-11-04 Thread Kristian Høgsberg Kristensen 
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 
---
 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. */
+
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);
-- 
2.6.2

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev