[Mesa-dev] [PATCH 06/10] i965: Lower the GLSL ES 3.00 pack/unpack operations

[Ian Romanick]

On 01/10/2013 01:51 PM, Chad Versace wrote:
> On 01/10/2013 10:39 AM, Ian Romanick wrote:
>> On 01/10/2013 12:10 AM, Chad Versace wrote:
>>> On gen < 7, we fully lower all operations to arithmetic and bitwise
>>> operations.
>>>
>>> On gen >= 7, we fully lower the Snorm2x16 and Unorm2x16 operations, and
>>> partially lower the Half2x16 operations.
>>>
>>> Signed-off-by: Chad Versace <chad.versace at linux.intel.com>
>>> ---
>>>    src/glsl/lower_packing_builtins.cpp      |  1 +
>>>    src/mesa/drivers/dri/i965/brw_shader.cpp | 32 ++++++++++++++++++++++++++++++++
>>>    2 files changed, 33 insertions(+)
>>>
>>> diff --git a/src/glsl/lower_packing_builtins.cpp
>>> b/src/glsl/lower_packing_builtins.cpp
>>> index cd84084..f965a27 100644
>>> --- a/src/glsl/lower_packing_builtins.cpp
>>> +++ b/src/glsl/lower_packing_builtins.cpp
>>> @@ -1013,6 +1013,7 @@ private:
>>>             new(mem_ctx) ir_variable(glsl_type::vec2_type,
>>>                                      "tmp_split_pack_half_2x16_v",
>>>                                      ir_var_temporary);
>>> +      insert_instruction(v);
>>>          insert_instruction(
>>>             new(mem_ctx) ir_assignment(
>>>                new(mem_ctx) ir_dereference_variable(v),
>>
>> Shouldn't this hunk be in the previous patch?
>
> Right. My mistake.
>
>>> diff --git a/src/mesa/drivers/dri/i965/brw_shader.cpp
>>> b/src/mesa/drivers/dri/i965/brw_shader.cpp
>>> index 1e8d574..65f8e7d 100644
>>> --- a/src/mesa/drivers/dri/i965/brw_shader.cpp
>>> +++ b/src/mesa/drivers/dri/i965/brw_shader.cpp
>>> @@ -75,6 +75,34 @@ brw_shader_precompile(struct gl_context *ctx, struct
>>> gl_shader_program *prog)
>>>       return true;
>>>    }
>>>
>>> +static void
>>> +brw_lower_packing_builtins(struct brw_context *brw,
>>> +                           gl_shader_type shader_type,
>>> +                           exec_list *ir)
>>> +{
>>> +   int ops = LOWER_PACK_SNORM_2x16
>>> +           | LOWER_UNPACK_SNORM_2x16
>>> +           | LOWER_PACK_UNORM_2x16
>>> +           | LOWER_UNPACK_UNORM_2x16;
>>> +
>>> +   if (brw->intel.gen >= 7) {
>>> +      switch (shader_type) {
>>> +      case MESA_SHADER_FRAGMENT:
>>> +         /* Scalarize the these operations. */
>>> +         ops |= LOWER_PACK_HALF_2x16_TO_SPLIT
>>> +             |  LOWER_UNPACK_HALF_2x16_TO_SPLIT;
>>> +         break;
>>
>> Do we think other shader types are going to need similar treatment? Otherwise an
>> if-statement would be better.
>
> Only SOA code will require splitting the Half2x16 functions. That rules out the
> vs and gs, at least for now.
>
> I don't really care either way about switch-vs-if. So, if you think it's easier
> to read with an if-statement, I can do that. It'll look like this:
>
> if (brw->intel.gen >= 7 && shader_type == MESA_SHADER_FRAGMENT) {
>     ops |= ...;
> } else {
>     ops |= ...;
> }

That's different from what you currently have.  What you currently have is

    if (brw->intel.gen >= 7) {
       if (shader_type == MESA_SHADER_FRAGMENT) {
          ops |= ...;
       }
    } else {
       ops |= ...;
    }

Right?  I think these if-statements with the comment about SOA code is good.