Re: [Mesa-dev] [RFC] i965/fs: Generalize grf127 hack to dispatch_width >
> > diff --git a/src/intel/compiler/brw_fs_reg_allocate.cpp > > b/src/intel/compiler/brw_fs_reg_allocate.cpp > > index 59e047483c0..417ddeba09c 100644 > > --- a/src/intel/compiler/brw_fs_reg_allocate.cpp > > +++ b/src/intel/compiler/brw_fs_reg_allocate.cpp > > @@ -549,7 +549,7 @@ fs_visitor::assign_regs(bool allow_spilling, bool > > spill_all) > >if (devinfo->gen >= 7) > > node_count += BRW_MAX_GRF - GEN7_MRF_HACK_START; > >int grf127_send_hack_node = node_count; > > - if (devinfo->gen >= 8 && dispatch_width == 8) > > + if (devinfo->gen >= 8 && dispatch_width >= 8) > > dispatch_width is always >= 8. Yes, I was so focused on the other bits that completely messed up this one. Thanks, Caio ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [RFC] i965/fs: Generalize grf127 hack to dispatch_width > 8
Hi,
Thanks for the explanations :-)
> > - ra_set_node_reg(g, grf127_send_hack_node, 127);
> > + ra_set_node_reg(g, grf127_send_hack_node, 128 - reg_width);
>
> This configuration is more restrictive than needed. The original code
> just avoids any register with any length that uses the physical register
> grf127. Your code works for SIMD16, but as you are setting conflicts
> with grf126 in SIMD16, you are forbidding the use of grf125 using with
> regsize=2, and the same with grf123 with size 4, when this options never
> use grf127. You don't need to take care of the reg_width here, just
> about which physical register you can not use.
That was my first attempt, but I think it was failing because of the
mistake below.
> >foreach_block_and_inst(block, fs_inst, inst, cfg) {
> > if (inst->is_send_from_grf() && inst->dst.file == VGRF) {
> > ra_add_node_interference(g, inst->dst.nr,
> > grf127_send_hack_node);
> >
>
> The issue here is that the unspill instructions aren't in the list of
> the is_send_from_grf. I thought we could update is_send_from_grf to
> include the read/write scratch operations but finally I think that it
> didn't have sense because the source at this point is an MRF that will
> be finally assigned to a GRF on Gen7+.
Yes. Reading more of the spilling code today I can see how this won't
work. I was somehow under the idea that the actual register choice
would be preserved under a spill, but if we are spilling is precisely
because we don't have proper register allocation.
> I've sent a patch with my solution that I think solves the case of
> unspill that is creating this problem, but maybe we need to think if
> there are more SEND instructions that could have this problem because of
> using the MRF as source.
Great! I'll take a look.
Thanks,
Caio
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [RFC] i965/fs: Generalize grf127 hack to dispatch_width > 8
El 11/07/18 a las 03:50, Caio Marcelo de Oliveira Filho escribió:
> Change the hack to always apply, adjusting the register number
> according to the dispatch_width.
>
> The original change assumed that given for dispatch_width > 8 we
> already prevent the overlap of source and destination for send, it
> would not be necessary to explicitly add an interference with a
> register that covers r127.
>
> The problem is that the code for spilling registers ends up generating
> scratch reads, that in Gen7+ will reuse the destination register,
> causing a send with both source and destination overlaping. So prevent
> r127 (or the overlapping wider register) to be used as destination for
> sends.
>
> This patch fixes piglit test
> tests/spec/arb_compute_shader/linker/bug-93840.shader_test.
>
> Fixes: 232ed898021 "i965/fs: Register allocator shoudn't use grf127 for sends
> dest"
> ---
>
> After more digging on the piglit failure, I came up with this
> patch. I'm still seeing crashes with for some shader-db executions
> (master have them too), but didn't have time today to drill into them
>
> src/intel/compiler/brw_fs_reg_allocate.cpp | 11 ---
> 1 file changed, 4 insertions(+), 7 deletions(-)
>
> diff --git a/src/intel/compiler/brw_fs_reg_allocate.cpp
> b/src/intel/compiler/brw_fs_reg_allocate.cpp
> index 59e047483c0..417ddeba09c 100644
> --- a/src/intel/compiler/brw_fs_reg_allocate.cpp
> +++ b/src/intel/compiler/brw_fs_reg_allocate.cpp
> @@ -549,7 +549,7 @@ fs_visitor::assign_regs(bool allow_spilling, bool
> spill_all)
> if (devinfo->gen >= 7)
>node_count += BRW_MAX_GRF - GEN7_MRF_HACK_START;
> int grf127_send_hack_node = node_count;
> - if (devinfo->gen >= 8 && dispatch_width == 8)
> + if (devinfo->gen >= 8 && dispatch_width >= 8)
>node_count ++;
> struct ra_graph *g =
>ra_alloc_interference_graph(compiler->fs_reg_sets[rsi].regs,
> node_count);
> @@ -656,7 +656,7 @@ fs_visitor::assign_regs(bool allow_spilling, bool
> spill_all)
>}
> }
>
> - if (devinfo->gen >= 8 && dispatch_width == 8) {
> + if (devinfo->gen >= 8 && dispatch_width >= 8) {
>/* At Intel Broadwell PRM, vol 07, section "Instruction Set Reference",
> * subsection "EUISA Instructions", Send Message (page 990):
> *
> @@ -665,12 +665,9 @@ fs_visitor::assign_regs(bool allow_spilling, bool
> spill_all)
> *
> * We are avoiding using grf127 as part of the destination of send
> * messages adding a node interference to the grf127_send_hack_node.
> - * This node has a fixed asignment to grf127.
> - *
> - * We don't apply it to SIMD16 because previous code avoids any
> register
> - * overlap between sources and destination.
> + * This node has a fixed assignment that overlaps with grf127.
> */
> - ra_set_node_reg(g, grf127_send_hack_node, 127);
> + ra_set_node_reg(g, grf127_send_hack_node, 128 - reg_width);
This configuration is more restrictive than needed. The original code
just avoids any register with any length that uses the physical register
grf127. Your code works for SIMD16, but as you are setting conflicts
with grf126 in SIMD16, you are forbidding the use of grf125 using with
regsize=2, and the same with grf123 with size 4, when this options never
use grf127. You don't need to take care of the reg_width here, just
about which physical register you can not use.
At brw_alloc_reg_set() you can check how the different registers are
defined using classes are used for different sizes. It also configures
the conflicts among the registers with different sizes and the physical
register.
So if at this point you create a node assigned to a physical register
you have conflicts with all the logical registers with any size that
overlap with it.
>foreach_block_and_inst(block, fs_inst, inst, cfg) {
> if (inst->is_send_from_grf() && inst->dst.file == VGRF) {
> ra_add_node_interference(g, inst->dst.nr, grf127_send_hack_node);
>
The issue here is that the unspill instructions aren't in the list of
the is_send_from_grf. I thought we could update is_send_from_grf to
include the read/write scratch operations but finally I think that it
didn't have sense because the source at this point is an MRF that will
be finally assigned to a GRF on Gen7+.
I've sent a patch with my solution that I think solves the case of
unspill that is creating this problem, but maybe we need to think if
there are more SEND instructions that could have this problem because of
using the MRF as source.
Chema
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [RFC] i965/fs: Generalize grf127 hack to dispatch_width >
On July 10, 2018 18:50:51 Caio Marcelo de Oliveira Filho
wrote:
Change the hack to always apply, adjusting the register number
according to the dispatch_width.
The original change assumed that given for dispatch_width > 8 we
already prevent the overlap of source and destination for send, it
would not be necessary to explicitly add an interference with a
register that covers r127.
The problem is that the code for spilling registers ends up generating
scratch reads, that in Gen7+ will reuse the destination register,
causing a send with both source and destination overlaping. So prevent
r127 (or the overlapping wider register) to be used as destination for
sends.
This patch fixes piglit test
tests/spec/arb_compute_shader/linker/bug-93840.shader_test.
Fixes: 232ed898021 "i965/fs: Register allocator shoudn't use grf127 for
sends dest"
---
After more digging on the piglit failure, I came up with this
patch. I'm still seeing crashes with for some shader-db executions
(master have them too), but didn't have time today to drill into them.
src/intel/compiler/brw_fs_reg_allocate.cpp | 11 ---
1 file changed, 4 insertions(+), 7 deletions(-)
diff --git a/src/intel/compiler/brw_fs_reg_allocate.cpp
b/src/intel/compiler/brw_fs_reg_allocate.cpp
index 59e047483c0..417ddeba09c 100644
--- a/src/intel/compiler/brw_fs_reg_allocate.cpp
+++ b/src/intel/compiler/brw_fs_reg_allocate.cpp
@@ -549,7 +549,7 @@ fs_visitor::assign_regs(bool allow_spilling, bool
spill_all)
if (devinfo->gen >= 7)
node_count += BRW_MAX_GRF - GEN7_MRF_HACK_START;
int grf127_send_hack_node = node_count;
- if (devinfo->gen >= 8 && dispatch_width == 8)
+ if (devinfo->gen >= 8 && dispatch_width >= 8)
dispatch_width is always >= 8.
node_count ++;
struct ra_graph *g =
ra_alloc_interference_graph(compiler->fs_reg_sets[rsi].regs, node_count);
@@ -656,7 +656,7 @@ fs_visitor::assign_regs(bool allow_spilling, bool
spill_all)
}
}
- if (devinfo->gen >= 8 && dispatch_width == 8) {
+ if (devinfo->gen >= 8 && dispatch_width >= 8) {
Same here
/* At Intel Broadwell PRM, vol 07, section "Instruction Set Reference",
* subsection "EUISA Instructions", Send Message (page 990):
*
@@ -665,12 +665,9 @@ fs_visitor::assign_regs(bool allow_spilling, bool
spill_all)
*
* We are avoiding using grf127 as part of the destination of send
* messages adding a node interference to the grf127_send_hack_node.
- * This node has a fixed asignment to grf127.
- *
- * We don't apply it to SIMD16 because previous code avoids any register
- * overlap between sources and destination.
+ * This node has a fixed assignment that overlaps with grf127.
*/
- ra_set_node_reg(g, grf127_send_hack_node, 127);
+ ra_set_node_reg(g, grf127_send_hack_node, 128 - reg_width);
foreach_block_and_inst(block, fs_inst, inst, cfg) {
if (inst->is_send_from_grf() && inst->dst.file == VGRF) {
ra_add_node_interference(g, inst->dst.nr, grf127_send_hack_node);
--
2.18.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
