[Mesa-dev] [PATCH 18/78] i965: Take is_scalar_shader_stage() method out to allow reuse
Jason Ekstrand
jason at jlekstrand.net
Wed Jul 22 09:37:56 PDT 2015
On Wed, Jul 22, 2015 at 4:26 AM, Eduardo Lima Mitev <elima at igalia.com> wrote:
> 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?
Looks fine to me.
>>>>> 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