[Mesa-dev] [PATCH 18/78] i965: Take is_scalar_shader_stage() method out to allow reuse
Jason Ekstrand
jason at jlekstrand.net
Mon Jul 13 04:38:24 PDT 2015
On Fri, Jul 10, 2015 at 8:53 AM, Eduardo Lima Mitev <elima at igalia.com> wrote:
> 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?
My primary issue was with having it based on a is_scalar_stage
function at all. It seems like a better long-term plan would be to
simply pass an is_scalar boolean into those lowering functions. That
said, I haven't taken a real hard look as to what calls those
functions and whether or not that's actually what we want either.
Make sense? If there's no better place to make that determination
further up the call chain, then this is fine for now.
--Jason
>>> 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
>>
>
More information about the mesa-dev
mailing list