[Mesa-dev] [PATCH 1/2] i965/vec4: Fix saturation errors when coalescing registers

[Jason Ekstrand]

On Fri, Aug 14, 2015 at 4:56 AM, Antia Puentes <apuentes at igalia.com> wrote:
> If the register types do not match and the instruction
> that contains the final destination is saturated, register
> coalescing generated non-equivalent code.
>
> This did not happen when using IR because types usually
> matched, but it is visible in nir-vec4.
>
> For example,
>    mov      vgrf7:D vgrf2:D
>    mov.sat  m4:F vgrf7:F
>
> is coalesced to:
>    mov.sat  m4:D vgrf2:D
>
> The patch prevents coalescing in such scenario, unless the
> instruction we want to coalesce into is a MOV. In that case,
> the patch sets the register types to the type of the final
> destination.
> ---
>  src/mesa/drivers/dri/i965/brw_vec4.cpp | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_vec4.cpp b/src/mesa/drivers/dri/i965/brw_vec4.cpp
> index f18915a..52982c3 100644
> --- a/src/mesa/drivers/dri/i965/brw_vec4.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_vec4.cpp
> @@ -1053,6 +1053,15 @@ vec4_visitor::opt_register_coalesce()
>                 }
>              }
>
> +            /* This doesn't handle saturation on the instruction we
> +             * want to coalesce away if the register types do not match.
> +             * But if scan_inst is a 'mov' we can fix the types later.
> +             */
> +            if (inst->saturate &&
> +                inst->dst.type != scan_inst->dst.type &&
> +                scan_inst->opcode != BRW_OPCODE_MOV)
> +               break;
> +
>              /* If we can't handle the swizzle, bail. */
>              if (!scan_inst->can_reswizzle(inst->dst.writemask,
>                                            inst->src[0].swizzle,
> @@ -1128,6 +1137,15 @@ vec4_visitor::opt_register_coalesce()
>                scan_inst->dst.file = inst->dst.file;
>                scan_inst->dst.reg = inst->dst.reg;
>                scan_inst->dst.reg_offset = inst->dst.reg_offset;
> +               if (inst->saturate &&
> +                   inst->dst.type != scan_inst->dst.type) {
> +                  /* If we have reached this point, scan_inst is a 'mov' and
> +                   * we can modify its register types to match the ones in inst.
> +                   * Otherwise, we could have an incorrect saturation result.
> +                   */
> +                  scan_inst->dst.type = inst->dst.type;
> +                  scan_inst->src[0].type = inst->src[0].type;

This is *almost* correct.  However, if scan_inst is a type-converting
move, we get

mov b:D, a:F
mov.sat c:F, b:F

this will map to

mov.sat c:F, a:F

which is obviously not correct.  Now, why anyone would want to do that
conversion is beyond me, but it's theoretically possible.  If we're
going to try and change the types to make the sat work, then we need
to also make sure that the two types match.  So the condition you
check should be something like

if (inst->saturate &&
    inst->dst.type != scan_inst->dst.type &&
    !(scan_inst->opcode == BRW_OPCODE_MOV &&
      scan_inst->dst.type == scan_inst->src[0].type)
   break;

Does that work?

With that changed,

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

Also, please add the following before pushing:

Cc: "10.6 11.0" <mesa-stable at lists.freedesktop.org>

> +               }
>                scan_inst->saturate |= inst->saturate;
>             }
>             scan_inst = (vec4_instruction *)scan_inst->next;
> --
> 2.1.0
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev