Re: [Mesa-dev] [PATCH 20/22] i965/vec4: Relax writemask condition in CSE

2018-03-07 Thread Kenneth Graunke 
On Tuesday, March 6, 2018 11:10:57 AM PST Ian Romanick wrote:
> On 03/05/2018 02:50 PM, Kenneth Graunke wrote:
> > On Friday, February 23, 2018 3:56:05 PM PST Ian Romanick wrote:
> >> From: Ian Romanick 
> >>
> >> If the previously seen instruction generates more fields than the new
> >> instruction, still allow CSE to happen.  This doesn't do much, but it
> >> also enables a couple more shaders in the next patch.  It helped quite a
> >> bit in another change series that I have (at least for now) abandoned.
> >>
> >> No changes on Skylake, Broadwell, Iron Lake or GM45.
> > 
> > Skylake and Broadwell sort of go without saying, they don't use this
> > compiler :)
> > 
> > It might be worth leaving some kind of comment (or renaming variables?)
> > in instructions_match() to indicate that 'a' is the generating
> > expression and 'b' is the second instance...since it's no longer
> > exactly symmetrical.
> 
> I thought about that... I just couldn't come up with any names that I
> actually liked.  Do you have any favorites? :)
> 
> > Either way,
> > Reviewed-by: Kenneth Graunke 

No, I don't really have any good suggestions either.  I'd be happy with
simply adding a function-level comment like this one:

/**
 * Checks if instructions match, exactly for sources, but loosely for
 * destination writemasks.
 *
 * \param 'a' is the generating expression from the AEB entry.
 * \param 'b' is the second occurrence of the expression that we're
 *considering eliminating.
 */

Feel free to adjust as you see fit.


signature.asc
Description: This is a digitally signed message part.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 20/22] i965/vec4: Relax writemask condition in CSE

2018-03-06 Thread Ian Romanick 
On 03/05/2018 02:50 PM, Kenneth Graunke wrote:
> On Friday, February 23, 2018 3:56:05 PM PST Ian Romanick wrote:
>> From: Ian Romanick 
>>
>> If the previously seen instruction generates more fields than the new
>> instruction, still allow CSE to happen.  This doesn't do much, but it
>> also enables a couple more shaders in the next patch.  It helped quite a
>> bit in another change series that I have (at least for now) abandoned.
>>
>> No changes on Skylake, Broadwell, Iron Lake or GM45.
> 
> Skylake and Broadwell sort of go without saying, they don't use this
> compiler :)
> 
> It might be worth leaving some kind of comment (or renaming variables?)
> in instructions_match() to indicate that 'a' is the generating
> expression and 'b' is the second instance...since it's no longer
> exactly symmetrical.

I thought about that... I just couldn't come up with any names that I
actually liked.  Do you have any favorites? :)

> Either way,
> Reviewed-by: Kenneth Graunke 



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

Re: [Mesa-dev] [PATCH 20/22] i965/vec4: Relax writemask condition in CSE

2018-03-05 Thread Kenneth Graunke 
On Friday, February 23, 2018 3:56:05 PM PST Ian Romanick wrote:
> From: Ian Romanick 
> 
> If the previously seen instruction generates more fields than the new
> instruction, still allow CSE to happen.  This doesn't do much, but it
> also enables a couple more shaders in the next patch.  It helped quite a
> bit in another change series that I have (at least for now) abandoned.
> 
> No changes on Skylake, Broadwell, Iron Lake or GM45.

Skylake and Broadwell sort of go without saying, they don't use this
compiler :)

It might be worth leaving some kind of comment (or renaming variables?)
in instructions_match() to indicate that 'a' is the generating
expression and 'b' is the second instance...since it's no longer
exactly symmetrical.

Either way,
Reviewed-by: Kenneth Graunke 


signature.asc
Description: This is a digitally signed message part.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH 20/22] i965/vec4: Relax writemask condition in CSE

2018-02-23 Thread Ian Romanick 
From: Ian Romanick 

If the previously seen instruction generates more fields than the new
instruction, still allow CSE to happen.  This doesn't do much, but it
also enables a couple more shaders in the next patch.  It helped quite a
bit in another change series that I have (at least for now) abandoned.

No changes on Skylake, Broadwell, Iron Lake or GM45.

Ivy Bridge and Haswell had similar results. (Ivy Bridge shown)
total instructions in shared programs: 11780295 -> 11780294 (<.01%)
instructions in affected programs: 302 -> 301 (-0.33%)
helped: 1
HURT: 0

total cycles in shared programs: 257308315 -> 257308313 (<.01%)
cycles in affected programs: 2074 -> 2072 (-0.10%)
helped: 1
HURT: 0

Sandy Bridge
total instructions in shared programs: 10506687 -> 10506686 (<.01%)
instructions in affected programs: 335 -> 334 (-0.30%)
helped: 1
HURT: 0

Signed-off-by: Ian Romanick 
---
 src/intel/compiler/brw_vec4_cse.cpp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/intel/compiler/brw_vec4_cse.cpp 
b/src/intel/compiler/brw_vec4_cse.cpp
index 2e65ef7..aa2f127 100644
--- a/src/intel/compiler/brw_vec4_cse.cpp
+++ b/src/intel/compiler/brw_vec4_cse.cpp
@@ -127,7 +127,7 @@ instructions_match(vec4_instruction *a, vec4_instruction *b)
   a->base_mrf == b->base_mrf &&
   a->header_size == b->header_size &&
   a->shadow_compare == b->shadow_compare &&
-  a->dst.writemask == b->dst.writemask &&
+  ((a->dst.writemask & b->dst.writemask) == a->dst.writemask) &&
   a->force_writemask_all == b->force_writemask_all &&
   a->size_written == b->size_written &&
   a->exec_size == b->exec_size &&
-- 
2.9.5

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