Re: [Mesa-dev] [PATCH] intel/fs: Do the grf127 hack on SIMD8 instructions in SIMD16 mode
If Matt concerns about the validation rule are solved this is.
Reviewed-by: Jose Maria Casanova Crespo
El 15/1/19 a las 17:58, Jason Ekstrand escribió:
> Previously, we only applied the fix to shaders with a dispatch mode of
> SIMD8 but the code it relies on for SIMD16 mode only applies to SIMD16
> instructions. If you have a SIMD8 instruction in a SIMD16 shader,
> neither would trigger and the restriction could still be hit.
>
> Cc: Jose Maria Casanova Crespo
> Fixes: 232ed8980217dd "i965/fs: Register allocator shoudn't use grf127..."
> ---
> src/intel/compiler/brw_fs_reg_allocate.cpp | 13 ++---
> 1 file changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/src/intel/compiler/brw_fs_reg_allocate.cpp
> b/src/intel/compiler/brw_fs_reg_allocate.cpp
> index 5db5242452e..ec743f9b5bf 100644
> --- a/src/intel/compiler/brw_fs_reg_allocate.cpp
> +++ b/src/intel/compiler/brw_fs_reg_allocate.cpp
> @@ -667,15 +667,14 @@ fs_visitor::assign_regs(bool allow_spilling, bool
> spill_all)
> * 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.
> + * We don't apply it to SIMD16 instructions because previous code
> avoids
> + * any register overlap between sources and destination.
> */
>ra_set_node_reg(g, grf127_send_hack_node, 127);
> - 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);
> - }
> + foreach_block_and_inst(block, fs_inst, inst, cfg) {
> + if (inst->exec_size < 16 && 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) {
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] intel/fs: Do the grf127 hack on SIMD8 instructions in SIMD16 mode
El 16/1/19 a las 0:55, Matt Turner escribió:
> On Tue, Jan 15, 2019 at 8:58 AM Jason Ekstrand wrote:
>>
>> Previously, we only applied the fix to shaders with a dispatch mode of
>> SIMD8 but the code it relies on for SIMD16 mode only applies to SIMD16
>> instructions. If you have a SIMD8 instruction in a SIMD16 shader,
>> neither would trigger and the restriction could still be hit.
>>
>> Cc: Jose Maria Casanova Crespo
>> Fixes: 232ed8980217dd "i965/fs: Register allocator shoudn't use grf127..."
>> ---
>> src/intel/compiler/brw_fs_reg_allocate.cpp | 13 ++---
>> 1 file changed, 6 insertions(+), 7 deletions(-)
>>
>> diff --git a/src/intel/compiler/brw_fs_reg_allocate.cpp
>> b/src/intel/compiler/brw_fs_reg_allocate.cpp
>> index 5db5242452e..ec743f9b5bf 100644
>> --- a/src/intel/compiler/brw_fs_reg_allocate.cpp.
>> +++ b/src/intel/compiler/brw_fs_reg_allocate.cpp
>> @@ -667,15 +667,14 @@ fs_visitor::assign_regs(bool allow_spilling, bool
>> spill_all)
>> * 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.
>> + * We don't apply it to SIMD16 instructions because previous code
>> avoids
>> + * any register overlap between sources and destination.
>> */
>>ra_set_node_reg(g, grf127_send_hack_node, 127);
>> - 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);
>> - }
>> + foreach_block_and_inst(block, fs_inst, inst, cfg) {
>> + if (inst->exec_size < 16 && inst->is_send_from_grf() &&
>> + inst->dst.file == VGRF)
>> +ra_add_node_interference(g, inst->dst.nr,
>> grf127_send_hack_node);
>>}
>>
>
> Did the code in brw_eu_validate.c catch the case you found?
>
> In fact, that code looks wrong:
>
> | (brw_inst_dst_da_reg_nr(devinfo, inst) +
> | brw_inst_rlen(devinfo, inst) > 127) &&
>
> I think > should be >=. And maybe we should have a separate case
> earlier that checks that dst_nr+rlen actually fits in registers, and
> then change > to just ==. FFS :(
This restriction only applies when we have a return register for the
SEND so rlen is >= 1. If we had had an ">=" 127, we would be raising an
exception for a legal destination register for a send in the cases like
grf126 with rlen = 1.
We could agree that another way of expressing it would be using
dst_nr+rlen == 128. But in any case something over 128 would mean that
something went wrong too. :)
I agree that it also makes sense to include a general check for dst_nr +
rlen <= 128 and the same for sources would make sense, although it isn't
possible for our register allocator to assign that not existing
combination it would be safer for changes of the register classes.
> Not sure what I was thinking letting that patch through without a unit
> test. I'll do that.
I should have taken a look at test_eu_validate.cpp
Regards,
Chema
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] intel/fs: Do the grf127 hack on SIMD8 instructions in SIMD16 mode
On January 15, 2019 17:55:31 Matt Turner wrote:
On Tue, Jan 15, 2019 at 8:58 AM Jason Ekstrand wrote:
Previously, we only applied the fix to shaders with a dispatch mode of
SIMD8 but the code it relies on for SIMD16 mode only applies to SIMD16
instructions. If you have a SIMD8 instruction in a SIMD16 shader,
neither would trigger and the restriction could still be hit.
Cc: Jose Maria Casanova Crespo
Fixes: 232ed8980217dd "i965/fs: Register allocator shoudn't use grf127..."
---
src/intel/compiler/brw_fs_reg_allocate.cpp | 13 ++---
1 file changed, 6 insertions(+), 7 deletions(-)
diff --git a/src/intel/compiler/brw_fs_reg_allocate.cpp
b/src/intel/compiler/brw_fs_reg_allocate.cpp
index 5db5242452e..ec743f9b5bf 100644
--- a/src/intel/compiler/brw_fs_reg_allocate.cpp.
+++ b/src/intel/compiler/brw_fs_reg_allocate.cpp
@@ -667,15 +667,14 @@ fs_visitor::assign_regs(bool allow_spilling, bool
spill_all)
* 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.
+ * We don't apply it to SIMD16 instructions because previous code avoids
+ * any register overlap between sources and destination.
*/
ra_set_node_reg(g, grf127_send_hack_node, 127);
- 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);
- }
+ foreach_block_and_inst(block, fs_inst, inst, cfg) {
+ if (inst->exec_size < 16 && inst->is_send_from_grf() &&
+ inst->dst.file == VGRF)
+ra_add_node_interference(g, inst->dst.nr, grf127_send_hack_node);
}
Did the code in brw_eu_validate.c catch the case you found?
Yes, it did. It was a fairly simple case; it just occurred as a SIMD8
instruction in a SIMD16 program.
In fact, that code looks wrong:
| (brw_inst_dst_da_reg_nr(devinfo, inst) +
| brw_inst_rlen(devinfo, inst) > 127) &&
I think > should be >=. And maybe we should have a separate case
earlier that checks that dst_nr+rlen actually fits in registers, and
then change > to just ==. FFS :(
Not sure what I was thinking letting that patch through without a unit
test. I'll do that.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] intel/fs: Do the grf127 hack on SIMD8 instructions in SIMD16 mode
On Tue, Jan 15, 2019 at 8:58 AM Jason Ekstrand wrote:
>
> Previously, we only applied the fix to shaders with a dispatch mode of
> SIMD8 but the code it relies on for SIMD16 mode only applies to SIMD16
> instructions. If you have a SIMD8 instruction in a SIMD16 shader,
> neither would trigger and the restriction could still be hit.
>
> Cc: Jose Maria Casanova Crespo
> Fixes: 232ed8980217dd "i965/fs: Register allocator shoudn't use grf127..."
> ---
> src/intel/compiler/brw_fs_reg_allocate.cpp | 13 ++---
> 1 file changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/src/intel/compiler/brw_fs_reg_allocate.cpp
> b/src/intel/compiler/brw_fs_reg_allocate.cpp
> index 5db5242452e..ec743f9b5bf 100644
> --- a/src/intel/compiler/brw_fs_reg_allocate.cpp.
> +++ b/src/intel/compiler/brw_fs_reg_allocate.cpp
> @@ -667,15 +667,14 @@ fs_visitor::assign_regs(bool allow_spilling, bool
> spill_all)
> * 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.
> + * We don't apply it to SIMD16 instructions because previous code
> avoids
> + * any register overlap between sources and destination.
> */
>ra_set_node_reg(g, grf127_send_hack_node, 127);
> - 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);
> - }
> + foreach_block_and_inst(block, fs_inst, inst, cfg) {
> + if (inst->exec_size < 16 && inst->is_send_from_grf() &&
> + inst->dst.file == VGRF)
> +ra_add_node_interference(g, inst->dst.nr, grf127_send_hack_node);
>}
>
Did the code in brw_eu_validate.c catch the case you found?
In fact, that code looks wrong:
| (brw_inst_dst_da_reg_nr(devinfo, inst) +
| brw_inst_rlen(devinfo, inst) > 127) &&
I think > should be >=. And maybe we should have a separate case
earlier that checks that dst_nr+rlen actually fits in registers, and
then change > to just ==. FFS :(
Not sure what I was thinking letting that patch through without a unit
test. I'll do that.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
