Samuel Iglesias Gonsálvez <sigles...@igalia.com> writes:
> From: Iago Toral Quiroga <ito...@igalia.com>
>
> We should not offset into them based on the relative offset of
> our source and the destination of the instruction we are copy
> propagating from, so we don't turn this:
>
> mov(16) vgrf6:F, vgrf7+0.0<0>:F
> (...)
> load_payload(8) vgrf28:F, vgrf6+1.0:F 2ndhalf
> mov(8) vgrf29:DF, vgrf28:F 2ndhalf
>
> into:
>
> mov(16) vgrf6:F, vgrf7+0.0<0>:F
> (...)
> load_payload(8) vgrf28:F, vgrf7+1.0<0>:F 2ndhalf
> mov(8) vgrf29:DF, vgrf7+1.0<0>:F 2ndhalf
>
> and instead we do this:
>
> mov(16) vgrf6:F, vgrf7+0.0<0>:F
> (...)
> load_payload(8) vgrf28:F, vgrf7+0.0<0>:F 2ndhalf
> mov(8) vgrf29:DF, vgrf7+0.0<0>:F 2ndhalf
> ---
> src/mesa/drivers/dri/i965/brw_fs_copy_propagation.cpp | 16 +++++++++++++---
> 1 file changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_fs_copy_propagation.cpp
> b/src/mesa/drivers/dri/i965/brw_fs_copy_propagation.cpp
> index becc8bc..9147e60 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs_copy_propagation.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs_copy_propagation.cpp
> @@ -460,10 +460,20 @@ fs_visitor::try_copy_propagate(fs_inst *inst, int arg,
> acp_entry *entry)
> * parts of vgrfs so we have to do some reg_offset magic.
> */
>
> - /* Compute the offset of inst->src[arg] relative to inst->dst */
> + /* Compute the offset of inst->src[arg] relative to inst->dst
> + *
> + * If the source we are copy propagating from has a stride of 0,
> then
> + * we must not offset into it based on the offset of our source
> + * relative to entry->dst
> + */
> assert(entry->dst.subreg_offset == 0);
> - int rel_offset = inst->src[arg].reg_offset - entry->dst.reg_offset;
> - int rel_suboffset = inst->src[arg].subreg_offset;
> + int rel_offset, rel_suboffset;
> + if (entry->src.stride != 0) {
> + rel_offset = inst->src[arg].reg_offset - entry->dst.reg_offset;
> + rel_suboffset = inst->src[arg].subreg_offset;
> + } else {
> + rel_offset = rel_suboffset = 0;
> + }Sigh, this fixes the problem for stride == 0 but leaves the logic broken in the general case. Turns out I came across the same problem with SIMD32 and came up with the fix below that should work regardless of the stride value... > > /* Compute the final register offset (in bytes) */ > int offset = entry->src.reg_offset * 32 + entry->src.subreg_offset; > -- > 2.5.0 > > _______________________________________________ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
From 325c80c10ea9028271116094bf8b674ffc0eb9f2 Mon Sep 17 00:00:00 2001
From: Francisco Jerez <curroje...@riseup.net>
Date: Mon, 25 Apr 2016 15:40:05 -0700
Subject: [PATCH] i965/fs: Fix propagation of copies with strided source.
This has likely been broken since we started propagating copies not
matching the offset of the instruction exactly
(1728e74957a62b1b4b9fbb62a7de2c12b77c8a75). The copy source stride
needs to be taken into account to find out the offset at the origin
that corresponds to the offset at the destination of the copy which is
being read by the instruction. This has led to program miscompilation
on both my SIMD32 branch and Igalia's FP64 branch.
---
.../drivers/dri/i965/brw_fs_copy_propagation.cpp | 30 ++++++++++++++--------
1 file changed, 20 insertions(+), 10 deletions(-)
diff --git a/src/mesa/drivers/dri/i965/brw_fs_copy_propagation.cpp b/src/mesa/drivers/dri/i965/brw_fs_copy_propagation.cpp
index 3c702d8..83791bf 100644
--- a/src/mesa/drivers/dri/i965/brw_fs_copy_propagation.cpp
+++ b/src/mesa/drivers/dri/i965/brw_fs_copy_propagation.cpp
@@ -460,16 +460,26 @@ fs_visitor::try_copy_propagate(fs_inst *inst, int arg, acp_entry *entry)
* parts of vgrfs so we have to do some reg_offset magic.
*/
- /* Compute the offset of inst->src[arg] relative to inst->dst */
- assert(entry->dst.subreg_offset == 0);
- int rel_offset = inst->src[arg].reg_offset - entry->dst.reg_offset;
- int rel_suboffset = inst->src[arg].subreg_offset;
-
- /* Compute the final register offset (in bytes) */
- int offset = entry->src.reg_offset * 32 + entry->src.subreg_offset;
- offset += rel_offset * 32 + rel_suboffset;
- inst->src[arg].reg_offset = offset / 32;
- inst->src[arg].subreg_offset = offset % 32;
+ /* Compute the offset of inst->src[arg] relative to entry->dst */
+ const unsigned rel_offset = (inst->src[arg].reg_offset
+ - entry->dst.reg_offset) * REG_SIZE +
+ inst->src[arg].subreg_offset;
+
+ /* Compute the first component of the copy that the instruction is
+ * reading, and the base byte offset within that component.
+ */
+ assert(entry->dst.subreg_offset == 0 && entry->dst.stride == 1);
+ const unsigned component = rel_offset / type_sz(entry->dst.type);
+ const unsigned suboffset = rel_offset % type_sz(entry->dst.type);
+
+ /* Calculate the byte offset at the origin of the copy of the given
+ * component and suboffset.
+ */
+ const unsigned offset = suboffset +
+ component * entry->src.stride * type_sz(entry->src.type) +
+ entry->src.reg_offset * REG_SIZE + entry->src.subreg_offset;
+ inst->src[arg].reg_offset = offset / REG_SIZE;
+ inst->src[arg].subreg_offset = offset % REG_SIZE;
}
break;
--
2.7.3
signature.asc
Description: PGP signature
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
