[Mesa-dev] [PATCH 18/78] i965: Take is_scalar_shader_stage() method out to allow reuse
Eduardo Lima Mitev
elima at igalia.com
Wed Jul 22 04:26:08 PDT 2015
On 07/13/2015 01:38 PM, Jason Ekstrand wrote:
> 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.
>
Yes, I like the idea of passing the is_scalar boolean to
brw_create_nir() directly, so that this decision is moved up to a higher
context. By doing this we also leave the is_scalar_shader_stage()
function private to brw_shader.cpp, meaning this patch is not relevant
anymore and we leave that function untouched.
A patch illustrating these changes is at:
https://github.com/Igalia/mesa/commit/9e7bc62d43eaa30de005c40b91551ad113c4065c
In the future, if we want to select shader type (scalar vs. vector)
based on dynamic info, the logic that needs changing is contained in
brw_shader. But this patch doesn't address that yet, lacking a more
defined use-case. My (wild) idea would be to have a
"preferred_shader_type" variable in brw_context for example, that can be
modifiable dynamically at any point, and will instruct the link phase to
use scalar or vec4 if possible (depending on gen, stage, etc).
What do you think?
>>>> 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