[Mesa-dev] [PATCH 2/2] i965/fs: Consider type mismatches in saturate propagation.

[Jason Ekstrand]

On Wed, Oct 14, 2015 at 11:30 AM, Matt Turner <mattst88 at gmail.com> wrote:
> NIR considers bcsel to produce and consume unsigned types, leading to
> SEL instructions operating on unsigned types when the data is really
> floating-point. Previous to this patch, saturate propagation would
> happily transform
>
>    (+f0) sel      g20:UD, g30:UD, g40:UD
>          mov.sat  g50:F,  g20:F
>
> into
>
>    (+f0) sel.sat  g20:UD, g30:UD, g40:UD
>          mov      g50:F,  g20:F
>
> But since the meaning of .sat is dependent on the type of the
> destination register, this is not valid.
>
> Instead, allow saturate propagation to change the types of dest/source
> on instructions that are simply copying data in order to propagate the
> saturate modifier.
>
> Fixes bad code gen in 158 programs.
> ---
>  src/mesa/drivers/dri/i965/brw_fs_saturate_propagation.cpp | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_fs_saturate_propagation.cpp b/src/mesa/drivers/dri/i965/brw_fs_saturate_propagation.cpp
> index e406c28..8792a8c 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs_saturate_propagation.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs_saturate_propagation.cpp
> @@ -52,11 +52,12 @@ opt_saturate_propagation_local(fs_visitor *v, bblock_t *block)
>        ip--;
>
>        if (inst->opcode != BRW_OPCODE_MOV ||
> +          !inst->saturate ||
>            inst->dst.file != GRF ||
> +          inst->dst.type != inst->src[0].type ||
>            inst->src[0].file != GRF ||
>            inst->src[0].abs ||
> -          inst->src[0].negate ||
> -          !inst->saturate)
> +          inst->src[0].negate)
>           continue;
>
>        int src_var = v->live_intervals->var_from_reg(inst->src[0]);
> @@ -65,7 +66,9 @@ opt_saturate_propagation_local(fs_visitor *v, bblock_t *block)
>        bool interfered = false;
>        foreach_inst_in_block_reverse_starting_from(fs_inst, scan_inst, inst, block) {
>           if (scan_inst->overwrites_reg(inst->src[0])) {
> -            if (scan_inst->is_partial_write())
> +            if (scan_inst->is_partial_write() ||
> +                (scan_inst->dst.type != inst->dst.type &&
> +                 !scan_inst->can_change_types()))
>                 break;
>
>              if (scan_inst->saturate) {
> @@ -73,6 +76,12 @@ opt_saturate_propagation_local(fs_visitor *v, bblock_t *block)
>                 progress = true;
>              } else if (src_end_ip <= ip || inst->dst.equals(inst->src[0])) {
>                 if (scan_inst->can_do_saturate()) {
> +                  if (scan_inst->dst.type != inst->dst.type) {

Please add an

assert(scan_inst->can_change_src_types());

With that added,

Reviewed-by: Jason Ekstrand <jason.ekstrand at intel.com>

> +                     scan_inst->dst.type = inst->dst.type;
> +                     for (int i = 0; i < scan_inst->sources; i++) {
> +                        scan_inst->src[i].type = inst->dst.type;
> +                     }
> +                  }
>                    scan_inst->saturate = true;
>                    inst->saturate = false;
>                    progress = true;
> --
> 2.4.9
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev