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

[Chad Versace]

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 |= ...;
}