[Mesa-dev] [PATCH 18/78] i965: Take is_scalar_shader_stage() method out to allow reuse

[Eduardo Lima Mitev]

On 06/30/2015 06:58 PM, Jason Ekstrand wrote:
> On Fri, Jun 26, 2015 at 1:06 AM, Eduardo Lima Mitev <elima at igalia.com> wrote:
>> This patch makes public the is_scalar_shader_stage() method in brw_shader, and
>> renames it to brw_compiler_is_scalar_shader_stage(). The plan is to later reuse it
>> in brw_nir, to enable/disable optimization passes depending on the type
>> of shader stage.
> 
> I'm not so sure that this is a good plan.  It assumes that whether we
> are doing a scalar or vec4 compile is based entirely on the shader
> stage and some static information (possibly based on environment
> variables).  Ken and I were talking around the office and we may want
> to use both SIMD4x2 and SIMD8 mode for geometry shaders depending on
> the number of inputs, etc.  This won't work in the given paradigm.
> 

If I understand correctly, what you propose is having a function to
dynamically choose the type of shader (scalar vs. vec4) when compiling
the shader, using not only gen and stage, but also actual application
data. I think this is a good idea and will allow experimenting with
different combinations of shaders with real input data.

However, I wonder if we this should be added later after more elaborated
thoughts on what exactly do we need and where to plug it. I have been
experimenting with a function following the use case you mentioned,
choosing shader backend based on inputs to a GS. But honestly it feels
like a blind guess from my side to what actually you and Ken have in mind.

Current patch basically reuses the function we already have to select
the shader, so what I propose is to move forward with it for the moment
(adding the missing MESA_SHADER_COMPUTE) and discuss how to extend it to
factor in dynamic data; or perhaps you can explain us your proposal with
a bit more detail.

WDYT?

>> The new method accepts a brw_compiler instead of a brw_context. This is done
>> for consistency, since the actual info we need (scalar_vs) is in brw_compiler,
>> and fetching in through brw_content->intelScreen->compiler seems like too
>> much indirection.
>>
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89580
>> ---
>>  src/mesa/drivers/dri/i965/brw_shader.cpp | 22 ++++++----------------
>>  src/mesa/drivers/dri/i965/brw_shader.h   | 13 +++++++++++++
>>  2 files changed, 19 insertions(+), 16 deletions(-)
>>
>> diff --git a/src/mesa/drivers/dri/i965/brw_shader.cpp b/src/mesa/drivers/dri/i965/brw_shader.cpp
>> index 0b53647..3b99046 100644
>> --- a/src/mesa/drivers/dri/i965/brw_shader.cpp
>> +++ b/src/mesa/drivers/dri/i965/brw_shader.cpp
>> @@ -182,19 +182,6 @@ brw_shader_precompile(struct gl_context *ctx,
>>     return true;
>>  }
>>
>> -static inline bool
>> -is_scalar_shader_stage(struct brw_context *brw, int stage)
>> -{
>> -   switch (stage) {
>> -   case MESA_SHADER_FRAGMENT:
>> -      return true;
>> -   case MESA_SHADER_VERTEX:
>> -      return brw->intelScreen->compiler->scalar_vs;
>> -   default:
>> -      return false;
>> -   }
>> -}
>> -
>>  static void
>>  brw_lower_packing_builtins(struct brw_context *brw,
>>                             gl_shader_stage shader_type,
>> @@ -205,7 +192,8 @@ brw_lower_packing_builtins(struct brw_context *brw,
>>             | LOWER_PACK_UNORM_2x16
>>             | LOWER_UNPACK_UNORM_2x16;
>>
>> -   if (is_scalar_shader_stage(brw, shader_type)) {
>> +   if (brw_compiler_is_scalar_shader_stage(brw->intelScreen->compiler,
>> +                                           shader_type)) {
>>        ops |= LOWER_UNPACK_UNORM_4x8
>>             | LOWER_UNPACK_SNORM_4x8
>>             | LOWER_PACK_UNORM_4x8
>> @@ -218,7 +206,8 @@ brw_lower_packing_builtins(struct brw_context *brw,
>>         * lowering is needed. For SOA code, the Half2x16 ops must be
>>         * scalarized.
>>         */
>> -      if (is_scalar_shader_stage(brw, shader_type)) {
>> +      if (brw_compiler_is_scalar_shader_stage(brw->intelScreen->compiler,
>> +                                              shader_type)) {
>>           ops |= LOWER_PACK_HALF_2x16_TO_SPLIT
>>               |  LOWER_UNPACK_HALF_2x16_TO_SPLIT;
>>        }
>> @@ -294,7 +283,8 @@ process_glsl_ir(struct brw_context *brw,
>>     do {
>>        progress = false;
>>
>> -      if (is_scalar_shader_stage(brw, shader->Stage)) {
>> +      if (brw_compiler_is_scalar_shader_stage(brw->intelScreen->compiler,
>> +                                              shader->Stage)) {
>>           brw_do_channel_expressions(shader->ir);
>>           brw_do_vector_splitting(shader->ir);
>>        }
>> diff --git a/src/mesa/drivers/dri/i965/brw_shader.h b/src/mesa/drivers/dri/i965/brw_shader.h
>> index b2c1a0b..cef2226 100644
>> --- a/src/mesa/drivers/dri/i965/brw_shader.h
>> +++ b/src/mesa/drivers/dri/i965/brw_shader.h
>> @@ -302,6 +302,19 @@ bool brw_cs_precompile(struct gl_context *ctx,
>>                         struct gl_shader_program *shader_prog,
>>                         struct gl_program *prog);
>>
>> +static inline bool
>> +brw_compiler_is_scalar_shader_stage(struct brw_compiler *compiler, int stage)
>> +{
>> +   switch (stage) {
>> +   case MESA_SHADER_FRAGMENT:
> 
> You need MESA_SHADER_COMPUTE here too.
> 
>> +      return true;
>> +   case MESA_SHADER_VERTEX:
>> +      return compiler->scalar_vs;
>> +   default:
>> +      return false;
>> +   }
>> +}
>> +
>>  #ifdef __cplusplus
>>  }
>>  #endif
>> --
>> 2.1.4
>>
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>