On Wed, 2016-01-27 at 12:29 -0800, Matt Turner wrote:
> On Wed, Jan 27, 2016 at 5:22 AM, Iago Toral <ito...@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 <ito...@igalia.com>
Iago
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
