Re: [Mesa-dev] [PATCH 03/23] i965/fs: Fix copy propagation of load payload for double operands

2016-05-11 Thread Samuel Iglesias Gonsálvez 
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA256



On 10/05/16 22:41, Francisco Jerez wrote:
> Samuel Iglesias Gonsálvez  writes:
> 
>> From: Iago Toral Quiroga 
>> 
>> Specifically, consider the size of the data type of the operand
>> to compute the number of registers written. --- 
>> src/mesa/drivers/dri/i965/brw_fs_copy_propagation.cpp | 2 +- 1
>> file changed, 1 insertion(+), 1 deletion(-)
>> 
>> 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
>> 9147e60..abc68c8 100644 ---
>> a/src/mesa/drivers/dri/i965/brw_fs_copy_propagation.cpp +++
>> b/src/mesa/drivers/dri/i965/brw_fs_copy_propagation.cpp @@ -775,7
>> +775,7 @@ fs_visitor::opt_copy_propagate_local(void
>> *copy_prop_ctx, bblock_t *block, int offset = 0; for (int i = 0;
>> i < inst->sources; i++) { int effective_width = i <
>> inst->header_size ? 8 : inst->exec_size; -int
>> regs_written = effective_width / 8; +int regs_written
>> = effective_width / 8 * type_sz(inst->src[i].type) / 4;
> 
> Please use 'effective_width * type_sz(...) / REG_SIZE' instead,
> they are not necessarily equivalent due to rounding and only the
> latter is correct when they aren't.  (The existing code still looks
> broken when the result is not an exact multiple of REG_SIZE but
> that probably belongs in a separate patch...)
> 
> With that fixed: Reviewed-by: Francisco Jerez
> 
> 

OK, thanks!

Sam

>> if (inst->src[i].file == VGRF) { acp_entry *entry =
>> ralloc(copy_prop_ctx, acp_entry); entry->dst = inst->dst; -- 
>> 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

iQIcBAEBCAAGBQJXMthdAAoJEH/0ujLxfcNDSH0P/j2ltsmobnG2R30mSD7l5Fi6
F09k6CFHaA1Wd6FllyABlLwxaj1H6dTeHl2AhQQGcZHFipcR1SQ6JxdnbqTvRyrc
I2FxqaZ+JRPWm+riBOVOJmAJIcN7kutIi8Yxy2nAMynmgQpvf1joBs4Q9v3DQBF4
DToc0Ge0dLKwH6uiO5E0ZwGDV3Qe2FLm5F7efd5mLn7NRS+lXjzsvh6/3xzmO4rs
pQB5KWZRFnjuaE/9lG55c8B2OeK8rrqxQgBAryUOi4qVHzQUl6Jf+swpWIY5H1QG
JV9zO1eB2iLIjvs7yjmjTPZPLm2bc2aauWr4NjuG07LVrpkBkMRgF3W2Sr39KvJb
ebbYOf5oXTYLiUJCidvyltihwnePBFnpfDqisdrsAduizl02ssTxbkR3Mv7iTDuR
h6aEanLfsry2eI2yDG3OU4uBwVRYEnTWzUlqxd6nDhP8v4/95nCeqWRdpkbS2g3H
tqLUS9sSXXkhIZp5unOT4EddTjyVBaslBhyN2pLkTiPCJExUcCZsVkAVYSTrY21b
2kOlLEqRI6sdY4cIzN0FYZ/0ZXuWYsTKuZy7FCvaYRUghm1sUzbww85syCIT7bMP
kwMVBrWviD+cAsDsbFEGCQgCeOCPhSMKfFjm+aiAeOI26IEbrgtx2OJqkhO83vf7
evrdPCnm3fk6LwXtDP3/
=UZwm
-END PGP SIGNATURE-
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 03/23] i965/fs: Fix copy propagation of load payload for double operands

2016-05-10 Thread Francisco Jerez 
Samuel Iglesias Gonsálvez  writes:

> From: Iago Toral Quiroga 
>
> Specifically, consider the size of the data type of the operand to compute
> the number of registers written.
> ---
>  src/mesa/drivers/dri/i965/brw_fs_copy_propagation.cpp | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> 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 9147e60..abc68c8 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs_copy_propagation.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs_copy_propagation.cpp
> @@ -775,7 +775,7 @@ fs_visitor::opt_copy_propagate_local(void *copy_prop_ctx, 
> bblock_t *block,
>   int offset = 0;
>   for (int i = 0; i < inst->sources; i++) {
>  int effective_width = i < inst->header_size ? 8 : 
> inst->exec_size;
> -int regs_written = effective_width / 8;
> +int regs_written = effective_width / 8 * 
> type_sz(inst->src[i].type) / 4;

Please use 'effective_width * type_sz(...) / REG_SIZE' instead, they are
not necessarily equivalent due to rounding and only the latter is
correct when they aren't.  (The existing code still looks broken when
the result is not an exact multiple of REG_SIZE but that probably
belongs in a separate patch...)

With that fixed:
Reviewed-by: Francisco Jerez 

>  if (inst->src[i].file == VGRF) {
> acp_entry *entry = ralloc(copy_prop_ctx, acp_entry);
> entry->dst = inst->dst;
> -- 
> 2.5.0
>
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev


signature.asc
Description: PGP signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 03/23] i965/fs: Fix copy propagation of load payload for double operands

2016-05-10 Thread Samuel Iglesias Gonsálvez 
On 04/05/16 00:51, Jordan Justen wrote:
> On 2016-05-03 05:21:52, Samuel Iglesias Gonsálvez wrote:
>> From: Iago Toral Quiroga 
>>
>> Specifically, consider the size of the data type of the operand to compute
>> the number of registers written.
>> ---
>>  src/mesa/drivers/dri/i965/brw_fs_copy_propagation.cpp | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> 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 9147e60..abc68c8 100644
>> --- a/src/mesa/drivers/dri/i965/brw_fs_copy_propagation.cpp
>> +++ b/src/mesa/drivers/dri/i965/brw_fs_copy_propagation.cpp
>> @@ -775,7 +775,7 @@ fs_visitor::opt_copy_propagate_local(void 
>> *copy_prop_ctx, bblock_t *block,
>>   int offset = 0;
>>   for (int i = 0; i < inst->sources; i++) {
>>  int effective_width = i < inst->header_size ? 8 : 
>> inst->exec_size;
>> -int regs_written = effective_width / 8;
>> +int regs_written = effective_width / 8 * 
>> type_sz(inst->src[i].type) / 4;
> 
> Line length.
> 

OK

> Should this be based on inst->dst?
> 

No, it shouldn't. When SHADER_OPCODE_LOAD_PAYLOAD is lowered, the
destination is retyped to source's type. Furthermore inside
LOAD_PAYLOAD(), regs_written is calculated with the type size of the
sources.

> type_sz(foo.type) will only ever be 4 or 8?
> 

I have not found any usage with a type suze less than 4. I added an
assert just in case this is changed in the future. No piglit regressions
with that assert in place.

Sam

> -Jordan
> 
>>  if (inst->src[i].file == VGRF) {
>> acp_entry *entry = ralloc(copy_prop_ctx, acp_entry);
>> entry->dst = inst->dst;
>> -- 
>> 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 03/23] i965/fs: Fix copy propagation of load payload for double operands

2016-05-03 Thread Jordan Justen 
On 2016-05-03 05:21:52, Samuel Iglesias Gonsálvez wrote:
> From: Iago Toral Quiroga 
> 
> Specifically, consider the size of the data type of the operand to compute
> the number of registers written.
> ---
>  src/mesa/drivers/dri/i965/brw_fs_copy_propagation.cpp | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> 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 9147e60..abc68c8 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs_copy_propagation.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs_copy_propagation.cpp
> @@ -775,7 +775,7 @@ fs_visitor::opt_copy_propagate_local(void *copy_prop_ctx, 
> bblock_t *block,
>   int offset = 0;
>   for (int i = 0; i < inst->sources; i++) {
>  int effective_width = i < inst->header_size ? 8 : 
> inst->exec_size;
> -int regs_written = effective_width / 8;
> +int regs_written = effective_width / 8 * 
> type_sz(inst->src[i].type) / 4;

Line length.

Should this be based on inst->dst?

type_sz(foo.type) will only ever be 4 or 8?

-Jordan

>  if (inst->src[i].file == VGRF) {
> acp_entry *entry = ralloc(copy_prop_ctx, acp_entry);
> entry->dst = inst->dst;
> -- 
> 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 03/23] i965/fs: Fix copy propagation of load payload for double operands

2016-05-03 Thread Samuel Iglesias Gonsálvez 
From: Iago Toral Quiroga 

Specifically, consider the size of the data type of the operand to compute
the number of registers written.
---
 src/mesa/drivers/dri/i965/brw_fs_copy_propagation.cpp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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 9147e60..abc68c8 100644
--- a/src/mesa/drivers/dri/i965/brw_fs_copy_propagation.cpp
+++ b/src/mesa/drivers/dri/i965/brw_fs_copy_propagation.cpp
@@ -775,7 +775,7 @@ fs_visitor::opt_copy_propagate_local(void *copy_prop_ctx, 
bblock_t *block,
  int offset = 0;
  for (int i = 0; i < inst->sources; i++) {
 int effective_width = i < inst->header_size ? 8 : inst->exec_size;
-int regs_written = effective_width / 8;
+int regs_written = effective_width / 8 * 
type_sz(inst->src[i].type) / 4;
 if (inst->src[i].file == VGRF) {
acp_entry *entry = ralloc(copy_prop_ctx, acp_entry);
entry->dst = inst->dst;
-- 
2.5.0

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev