On Wed, Jul 22, 2015 at 4:26 AM, Eduardo Lima Mitev <el...@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 <el...@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 <el...@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@lists.freedesktop.org >>>>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev >>>> >>> >> > _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
