[Mesa-dev] [PATCH 05/12] i965/fs: Switch from GLSL IR to NIR for un/packHalf2x16 lowering.

[Matt Turner]

On Wed, Jan 27, 2016 at 5:22 AM, Iago Toral <itoral at igalia.com> wrote:
> I think it would be a good idea to change the shortlog so it is clear
> that we are only talking about the scalarization aspect. There is still
> GLSL IR lowering for un/packHalf2x16 that remains in use after this
> patch, as made clear by the last hunk in this patch.

Yes, I will do that.

> On Mon, 2016-01-25 at 15:18 -0800, Matt Turner wrote:
>> ---
>>  src/mesa/drivers/dri/i965/brw_compiler.c                 |  2 ++
>>  src/mesa/drivers/dri/i965/brw_fs_channel_expressions.cpp |  4 ++++
>>  src/mesa/drivers/dri/i965/brw_link.cpp                   | 12 +-----------
>>  3 files changed, 7 insertions(+), 11 deletions(-)
>>
>> diff --git a/src/mesa/drivers/dri/i965/brw_compiler.c b/src/mesa/drivers/dri/i965/brw_compiler.c
>> index dbd5ad2..21fff1d 100644
>> --- a/src/mesa/drivers/dri/i965/brw_compiler.c
>> +++ b/src/mesa/drivers/dri/i965/brw_compiler.c
>> @@ -86,6 +86,8 @@ shader_perf_log_mesa(void *data, const char *fmt, ...)
>>
>>  static const struct nir_shader_compiler_options scalar_nir_options = {
>>     COMMON_OPTIONS,
>> +   .lower_pack_half_2x16 = true,
>> +   .lower_unpack_half_2x16 = true,
>>  };
>>
>>  static const struct nir_shader_compiler_options vector_nir_options = {
>> diff --git a/src/mesa/drivers/dri/i965/brw_fs_channel_expressions.cpp b/src/mesa/drivers/dri/i965/brw_fs_channel_expressions.cpp
>> index 21f0b70..566257c 100644
>> --- a/src/mesa/drivers/dri/i965/brw_fs_channel_expressions.cpp
>> +++ b/src/mesa/drivers/dri/i965/brw_fs_channel_expressions.cpp
>> @@ -72,6 +72,9 @@ channel_expressions_predicate(ir_instruction *ir)
>>        return false;
>>
>>     switch (expr->operation) {
>> +      case ir_unop_pack_half_2x16:
>> +         return false;
>> +
>>        /* these opcodes need to act on the whole vector,
>>         * just like texturing.
>>         */
>> @@ -162,6 +165,7 @@ ir_channel_expressions_visitor::visit_leave(ir_assignment *ir)
>>        return visit_continue;
>>
>>     switch (expr->operation) {
>> +      case ir_unop_pack_half_2x16:
>>        case ir_unop_interpolate_at_centroid:
>>        case ir_binop_interpolate_at_offset:
>>        case ir_binop_interpolate_at_sample:
>
> This opcode is still being handled below as:
>
> ...
>    case ir_unop_pack_half_2x16:
> ...
>       unreachable("should have been lowered");
>
> so that should probably go away in this patch.

Well, it is still unreachable and we don't have a default case.

> Also, don't we need to do all this same stuff for the unpack opcode?

No, channel_expressions only handles operations that have a vector
source, and only the pack operation takes a vector source.