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

[Iago Toral]

On Wed, 2016-01-27 at 12:29 -0800, Matt Turner wrote:
> 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.

Ah right... 

One question though, shouldn't we move the handling of
ir_unop_unpack_half_2x16 to the unreachable section with message "not
reached: expression operates on scalars only"? Right now it is in an
unreachable section with a message that suggests that it should've been
lowered, which is not really the case any more.

Reviewed-by: Iago Toral Quiroga <itoral at igalia.com>

Iago