Re: [Mesa-dev] [PATCH 02/23] i965/fs: fix copy propagation from sources with stride 0
-BEGIN PGP SIGNED MESSAGE- Hash: SHA256 On 10/05/16 22:57, Francisco Jerez wrote: > Samuel Iglesias Gonsálvezwrites: > >> From: Iago Toral Quiroga >> >> 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... > Thanks for the fix! We will use it instead and test it. Sam >> >> /* 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 > -BEGIN PGP SIGNATURE- Version: GnuPG v2 iQIcBAEBCAAGBQJXMti6AAoJEH/0ujLxfcNDsv4P/3EnZMjaOWthqw6A1i6IlT2C Z9nZlteua2V7guqC/yFIZQeX6UffSP9BzQ0ZUiCG/+9jg6zBfSzDNCEgWkLu/CxA Ihc8mcFh82Hxb8c6BVgduE2Ir08o34cYHMeojlVOwQVPavj3X/pbaeiYTyizriI4 NK+VjyfYBeOcfCN0uxSpOkhU+QMSN4XTJS7hly3+KmIWcY8ocR6Ymy+Dr1KFpn2u U0EdeQZ8HLKPhufMT3sTRx6sazBl39OeBfSLF6s2Gfc8mXX3obS7F4A+HIfXUqIg KJAt6fYU7aMSaxzx/kcjQ0WuZV+LJvTn/tgFPrZoBpXlwAdvVqPX2hiu+wUfOM6u nibSu9p5DdP5wxWMWPJSn8BNAcOW/eBrzLBd8jCYEnhANdStwQTC2gukI/M5pN81 Kdid/T4BfWHeAPJsK6N9+oxix+ecrs6VXwXVuraPgBwBG5LYYm8diYJ5tImnL7kh AcvlHywmJw8Y8ocnoQ+DXVb3amzygWctXrFmTQ1SW9ddLXK5YPrPrR9s5RtPpopl I4bFCKLRGWQpSxcMdu0OAMn+VaaToOEZHMar1NA+qxCT7hziSjGz33ypJBN+jW3P IUYGsNKcBX3Lq75u/C0y7IOc8q+YMm3kUIbPET05t4Zd7QeNP1WuOEepoB4yXe3Y zkDenIskGfWfU/l587kJ =GihL -END PGP SIGNATURE- ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 02/23] i965/fs: fix copy propagation from sources with stride 0
Samuel Iglesias Gonsálvezwrites: > From: Iago Toral Quiroga > > 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 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
Re: [Mesa-dev] [PATCH 02/23] i965/fs: fix copy propagation from sources with stride 0
On 03/05/16 20:30, Jordan Justen wrote: > On 2016-05-03 05:21:51, Samuel Iglesias Gonsálvez wrote: >> From: Iago Toral Quiroga>> >> 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 { > > Should the comment added above go here instead? > > Reviewed-by: Jordan Justen Right, I will move it to here. Thanks, Sam > >> +rel_offset = rel_suboffset = 0; >> + } >> >> /* 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 > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 02/23] i965/fs: fix copy propagation from sources with stride 0
On 2016-05-03 05:21:51, Samuel Iglesias Gonsálvez wrote: > From: Iago Toral Quiroga> > 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 { Should the comment added above go here instead? Reviewed-by: Jordan Justen > +rel_offset = rel_suboffset = 0; > + } > > /* 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 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 02/23] i965/fs: fix copy propagation from sources with stride 0
From: Iago Toral QuirogaWe 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; + } /* 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