Re: [Mesa-dev] [PATCH] i965/fs: unspills shoudn't use grf127 as dest since Gen8+
El 12/07/18 a las 03:23, Caio Marcelo de Oliveira Filho escribió:
> On Wed, Jul 11, 2018 at 06:03:05PM +0200, Jose Maria Casanova Crespo wrote:
>> At 232ed8980217dd65ab0925df28156f565b94b2e5 "i965/fs: Register allocator
>> shoudn't use grf127 for sends dest" we didn't take into account the case
>> of SEND instructions that are not send_from_grf. But since Gen7+ although
>> the backend still uses MRFs internally for sends they are finally asigned
>> to a GRFs.
>
> Typo "assigned".
Fixed.
>> In the case of unspills the backend assigns directly as source its
>> destination because it is suppose to be available. So we always have a
>> source-destination overlap. If the reg_allocator asigns registers that
>
> Typo "assigns".
Fixed.
>> include de grf127 we fail the validation rule that affects Gen8+
>
> Typo "the".
Fixed.
>> "r127 must not be used for return address when there is a src and dest
>> overlap in send instruction."
>>
>> So this patch activates the grf127_send_hack_node for Gen8+ and if we have
>> any register spilled we add interferences to the destination of the unspill
>> operations.
>
> I've spent some time testing why this patch was still not covering all
> the cases yet. The opt_bank_conflicts() optimization, that runs after
> the register allocation, was moving things around, causing the r127 to
> be used in the condition we were avoiding it.
>
> The code there already has the idea of not touching certain registers,
> so we should add something like
>
> /* At Intel Broadwell PRM, vol 07, section "Instruction Set Reference",
>* subsection "EUISA Instructions", Send Message (page 990):
>*
>* "r127 must not be used for return address when there is a src and
>* dest overlap in send instruction."
>*
>* Register allocation ensures that, so don't move 127 around to avoid
>* breaking that property.
>*/
> if (v->devinfo->gen >= 8)
> constrained[p.atom_of_reg(127)] = true;
>
> to function shader_reg_constraints() in
> brw_fs_bank_conflicts.cpp. This fixes the crashes I was seeing in
> shader-db.
>
> With the change to bank conflicts and the typos/style fixed, this
> patch is
Good finding. I like the clean and simple solution. At that point of
optimizing back conflicts I don't find a better way to don't mess with
grf127, although we are forbidding legal permutations when not SEND
instructions are in place. I've just putting the your code after the
constrains for reg0 and reg1.
I've also confirmed that that that I run a full shader-db without
crashes caused by this validation rule and the performance impact of the
patch doesn't seem to be too much taking into account that we are
avoiding generating instructions with undefined return values.
total instructions in shared programs: 14867211 -> 14867218 (<.01%)
instructions in affected programs: 5314 -> 5321 (0.13%)
helped: 1
HURT: 1
total cycles in shared programs: 537925161 -> 537923248 (<.01%)
cycles in affected programs: 44939136 -> 44937223 (<.01%)
helped: 10
HURT: 23
total spills in shared programs: 7789 -> 7790 (0.01%)
spills in affected programs: 107 -> 108 (0.93%)
helped: 0
HURT: 1
total fills in shared programs: 10555 -> 10557 (0.02%)
fills in affected programs: 155 -> 157 (1.29%)
helped: 0
HURT: 1
> Reviewed-by: Caio Marcelo de Oliveira Filho
Thanks for the review.
Chema
>
> Reviewed-by: Caio Marcelo de Oliveira Filho
>
>
>> + if (spilled_any_registers) {
>> + foreach_block_and_inst(block, fs_inst, inst, cfg) {
>> +if ((inst->opcode == SHADER_OPCODE_GEN7_SCRATCH_READ ||
>> +inst->opcode == SHADER_OPCODE_GEN4_SCRATCH_READ) &&
>> +inst->dst.file ==VGRF) {
>
> Missing space after the "==".
>
>> + ra_add_node_interference(g, inst->dst.nr,
>> grf127_send_hack_node);
>> +}
>> }
>>}
>> }
>>
>> +
>
> Extra newline?
>
>
> Thanks,
> Caio
> ___
> 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] i965/fs: unspills shoudn't use grf127 as dest since Gen8+
On Wed, Jul 11, 2018 at 06:03:05PM +0200, Jose Maria Casanova Crespo wrote:
> At 232ed8980217dd65ab0925df28156f565b94b2e5 "i965/fs: Register allocator
> shoudn't use grf127 for sends dest" we didn't take into account the case
> of SEND instructions that are not send_from_grf. But since Gen7+ although
> the backend still uses MRFs internally for sends they are finally asigned
> to a GRFs.
Typo "assigned".
> In the case of unspills the backend assigns directly as source its
> destination because it is suppose to be available. So we always have a
> source-destination overlap. If the reg_allocator asigns registers that
Typo "assigns".
> include de grf127 we fail the validation rule that affects Gen8+
Typo "the".
> "r127 must not be used for return address when there is a src and dest
> overlap in send instruction."
>
> So this patch activates the grf127_send_hack_node for Gen8+ and if we have
> any register spilled we add interferences to the destination of the unspill
> operations.
I've spent some time testing why this patch was still not covering all
the cases yet. The opt_bank_conflicts() optimization, that runs after
the register allocation, was moving things around, causing the r127 to
be used in the condition we were avoiding it.
The code there already has the idea of not touching certain registers,
so we should add something like
/* At Intel Broadwell PRM, vol 07, section "Instruction Set Reference",
* subsection "EUISA Instructions", Send Message (page 990):
*
* "r127 must not be used for return address when there is a src and
* dest overlap in send instruction."
*
* Register allocation ensures that, so don't move 127 around to avoid
* breaking that property.
*/
if (v->devinfo->gen >= 8)
constrained[p.atom_of_reg(127)] = true;
to function shader_reg_constraints() in
brw_fs_bank_conflicts.cpp. This fixes the crashes I was seeing in
shader-db.
With the change to bank conflicts and the typos/style fixed, this
patch is
Reviewed-by: Caio Marcelo de Oliveira Filho
> + if (spilled_any_registers) {
> + foreach_block_and_inst(block, fs_inst, inst, cfg) {
> +if ((inst->opcode == SHADER_OPCODE_GEN7_SCRATCH_READ ||
> +inst->opcode == SHADER_OPCODE_GEN4_SCRATCH_READ) &&
> +inst->dst.file ==VGRF) {
Missing space after the "==".
> + ra_add_node_interference(g, inst->dst.nr,
> grf127_send_hack_node);
> +}
> }
>}
> }
>
> +
Extra newline?
Thanks,
Caio
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH] i965/fs: unspills shoudn't use grf127 as dest since Gen8+
At 232ed8980217dd65ab0925df28156f565b94b2e5 "i965/fs: Register allocator
shoudn't use grf127 for sends dest" we didn't take into account the case
of SEND instructions that are not send_from_grf. But since Gen7+ although
the backend still uses MRFs internally for sends they are finally asigned
to a GRFs.
In the case of unspills the backend assigns directly as source its
destination because it is suppose to be available. So we always have a
source-destination overlap. If the reg_allocator asigns registers that
include de grf127 we fail the validation rule that affects Gen8+
"r127 must not be used for return address when there is a src and dest
overlap in send instruction."
So this patch activates the grf127_send_hack_node for Gen8+ and if we have
any register spilled we add interferences to the destination of the unspill
operations.
Found by Caio Marcelo de Oliveira Filho
Fixes piglit test tests/spec/arb_compute_shader/linker/bug-93840.shader_test
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107193
Fixes: 232ed89802 "i965/fs: Register allocator shoudn't use grf127 for sends
dest"
Cc: 18.1
Cc: Caio Marcelo de Oliveira Filho
Cc: Jason Ekstrand
---
src/intel/compiler/brw_fs_reg_allocate.cpp | 22 +-
1 file changed, 17 insertions(+), 5 deletions(-)
diff --git a/src/intel/compiler/brw_fs_reg_allocate.cpp
b/src/intel/compiler/brw_fs_reg_allocate.cpp
index 59e047483c0..3ea2e7547c6 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)
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) {
/* At Intel Broadwell PRM, vol 07, section "Instruction Set Reference",
* subsection "EUISA Instructions", Send Message (page 990):
*
@@ -671,13 +671,25 @@ fs_visitor::assign_regs(bool allow_spilling, bool
spill_all)
* overlap between sources and destination.
*/
ra_set_node_reg(g, grf127_send_hack_node, 127);
- 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);
+ if (dispatch_width == 8) {
+ 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);
+ }
+ }
+
+ if (spilled_any_registers) {
+ foreach_block_and_inst(block, fs_inst, inst, cfg) {
+if ((inst->opcode == SHADER_OPCODE_GEN7_SCRATCH_READ ||
+inst->opcode == SHADER_OPCODE_GEN4_SCRATCH_READ) &&
+inst->dst.file ==VGRF) {
+ ra_add_node_interference(g, inst->dst.nr,
grf127_send_hack_node);
+}
}
}
}
+
/* Debug of register spilling: Go spill everything. */
if (unlikely(spill_all)) {
int reg = choose_spill_reg(g);
--
2.17.1
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
