[Mesa-dev] [PATCH 29/57] i965/fs: Fix bogus sub-MRF offset calculation in compute-to-mrf.

[Iago Toral]

On Wed, 2016-09-07 at 18:48 -0700, Francisco Jerez wrote:
> The 'scan_inst->dst.offset % REG_SIZE' term in the final
> 'scan_inst->dst.offset' calculation is obviously bogus.  The offset
> from the start of the copy destination register 'inst->dst' where the
> destination of the generating instruction 'scan_inst' would be
> written
> to (before compute-to-mrf runs) is just the offset of 'scan_inst-
> >dst'
> relative to the source of the copy instruction (AKA rel_offset in the
> code below).
> ---
>  src/mesa/drivers/dri/i965/brw_fs.cpp | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp
> b/src/mesa/drivers/dri/i965/brw_fs.cpp
> index 5b7ca98..703340b 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
> @@ -2797,15 +2797,15 @@ fs_visitor::compute_to_mrf()
>                 inst->src[0], scan_inst->dst, DIV_ROUND_UP(scan_inst-
> >size_written,
>                                                            REG_SIZE))
> ;
>  
> -            const unsigned rel_offset = (reg_offset(scan_inst->dst)
> -
> -                                         reg_offset(inst->src[0])) /
> REG_SIZE;
> +            const unsigned rel_offset = reg_offset(scan_inst->dst) -
> +                                        reg_offset(inst->src[0]);
>  
>              if (inst->dst.nr & BRW_MRF_COMPR4) {
>                 /* Apply the same address transformation done by the
> hardware
>                  * for COMPR4 MRF writes.
>                  */
> -               assert(rel_offset < 2);
> -               scan_inst->dst.nr = inst->dst.nr + rel_offset * 4;
> +               assert(rel_offset < 2 * REG_SIZE);
> +               scan_inst->dst.nr = inst->dst.nr + rel_offset /
> REG_SIZE * 4;
>  
>                 /* Clear the COMPR4 bit if the generating instruction
> is not
>                  * compressed.
> @@ -2817,11 +2817,11 @@ fs_visitor::compute_to_mrf()
>                 /* Calculate the MRF number the result of this
> instruction is
>                  * ultimately written to.
>                  */
> -               scan_inst->dst.nr = inst->dst.nr + rel_offset;
> +               scan_inst->dst.nr = inst->dst.nr + rel_offset /
> REG_SIZE;
>              }
>  
>              scan_inst->dst.file = MRF;
> -            scan_inst->dst.offset = inst->dst.offset + scan_inst-
> >dst.offset % REG_SIZE;
> +            scan_inst->dst.offset = inst->dst.offset + rel_offset %
> REG_SIZE;
>              scan_inst->saturate |= inst->saturate;
>              if (!regs_left)
>                 break;

Ok, this makes a lot more sense :), maybe squash this and the previous
one together? I found the previous one quite confusing because it seems
to be doing something wrong that is then fixed here.