Re: [Mesa-dev] [PATCH 1/2] i965: remove cache_aux_free_func array
On 21 October 2015 at 22:44, Matt Turner wrote:
> On Wed, Oct 21, 2015 at 2:16 PM, Emil Velikov
> wrote:
>> On 21 October 2015 at 21:33, Kenneth Graunke wrote:
>>> On Monday, October 19, 2015 02:54:56 PM Emil Velikov wrote:
Ping on these two trivial patches ?
-Emil
>>>
>>> Oh, sorry, I thought I'd sent R-bs for these...
>>>
>>> Both are
>>> Reviewed-by: Kenneth Graunke
>> Thanks Ken. I was wondering if people looked at them and went "meh ...
>> too small, we need something beefier" :-P
>>
>> And now ... some C++ questions. I realise that templates (or is it
>> only STL?) are out of the question, but how about
>> - Initialization upon object declaration, rather than copy
>> constructors ? Rather trivial yet we have thousands of
>
> You're talking about things like this?
>
>fs_reg reg = fs_reg(...)
>
> I seem to remember trying at one point to replace those with just
>
>fs_reg reg(...)
>
> and found that it made absolutely no difference in the compiled code.
>
> The second's probably preferable if for no other reason than it's
> shorter, but I don't know that there's anything to be gained...
>
That's precisely what I meant.
>> duplicated/wasted CPU cycles due to it. One example is the memset()
>> from {fs,src,dst}_reg. Does the compiler squash/optimize those for us
>> ?
>
> Not sure. Experiments and data are welcome.
>
> I think people preferred the memset because there wasn't an
> alternative safe way of ensuring everything was initialized? Not sure.
>
Ack. I'll give it a few quick tests.
>> - Where is the line about "big enough to pass as reference" rather
>> than a copy for i965 ? It seems that older code(?) and extremely
>> common things such as the *_reg are passed around as copies.
>
> We want to use const references for dst_reg/src_reg/fs_reg where
> possible (see commit e58992aed and the three immediately before it for
> data). I don't think there should be many more instances of this.
>
I think I've saw a few more cases where we can do the same. I'll send
patches as I come across them.
> brw_reg is 8 bytes, so there's no reason to pass it as anything but by value.
Indeed.
Thanks
Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/2] i965: remove cache_aux_free_func array
On Wed, Oct 21, 2015 at 2:16 PM, Emil Velikov wrote:
> On 21 October 2015 at 21:33, Kenneth Graunke wrote:
>> On Monday, October 19, 2015 02:54:56 PM Emil Velikov wrote:
>>> Ping on these two trivial patches ?
>>>
>>> -Emil
>>
>> Oh, sorry, I thought I'd sent R-bs for these...
>>
>> Both are
>> Reviewed-by: Kenneth Graunke
> Thanks Ken. I was wondering if people looked at them and went "meh ...
> too small, we need something beefier" :-P
>
> And now ... some C++ questions. I realise that templates (or is it
> only STL?) are out of the question, but how about
> - Initialization upon object declaration, rather than copy
> constructors ? Rather trivial yet we have thousands of
You're talking about things like this?
fs_reg reg = fs_reg(...)
I seem to remember trying at one point to replace those with just
fs_reg reg(...)
and found that it made absolutely no difference in the compiled code.
The second's probably preferable if for no other reason than it's
shorter, but I don't know that there's anything to be gained...
> duplicated/wasted CPU cycles due to it. One example is the memset()
> from {fs,src,dst}_reg. Does the compiler squash/optimize those for us
> ?
Not sure. Experiments and data are welcome.
I think people preferred the memset because there wasn't an
alternative safe way of ensuring everything was initialized? Not sure.
> - Where is the line about "big enough to pass as reference" rather
> than a copy for i965 ? It seems that older code(?) and extremely
> common things such as the *_reg are passed around as copies.
We want to use const references for dst_reg/src_reg/fs_reg where
possible (see commit e58992aed and the three immediately before it for
data). I don't think there should be many more instances of this.
brw_reg is 8 bytes, so there's no reason to pass it as anything but by value.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/2] i965: remove cache_aux_free_func array
On 21 October 2015 at 21:33, Kenneth Graunke wrote:
> On Monday, October 19, 2015 02:54:56 PM Emil Velikov wrote:
>> Ping on these two trivial patches ?
>>
>> -Emil
>
> Oh, sorry, I thought I'd sent R-bs for these...
>
> Both are
> Reviewed-by: Kenneth Graunke
Thanks Ken. I was wondering if people looked at them and went "meh ...
too small, we need something beefier" :-P
And now ... some C++ questions. I realise that templates (or is it
only STL?) are out of the question, but how about
- Initialization upon object declaration, rather than copy
constructors ? Rather trivial yet we have thousands of
duplicated/wasted CPU cycles due to it. One example is the memset()
from {fs,src,dst}_reg. Does the compiler squash/optimize those for us
?
- Where is the line about "big enough to pass as reference" rather
than a copy for i965 ? It seems that older code(?) and extremely
common things such as the *_reg are passed around as copies.
Cheers,
Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/2] i965: remove cache_aux_free_func array
On Monday, October 19, 2015 02:54:56 PM Emil Velikov wrote: > Ping on these two trivial patches ? > > -Emil Oh, sorry, I thought I'd sent R-bs for these... Both are Reviewed-by: Kenneth Graunke signature.asc Description: This is a digitally signed message part. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/2] i965: remove cache_aux_free_func array
Ping on these two trivial patches ?
-Emil
On 7 October 2015 at 12:38, Emil Velikov wrote:
> There is only one function that can be called, which is well known at
> compilation time.
>
> The abstraction used here seems unnecessary, so let's use a direct call
> to brw_stage_prog_data_free() when appropriate, cut down the size of
> struct brw_cache.
>
> Signed-off-by: Emil Velikov
> ---
> src/mesa/drivers/dri/i965/brw_context.h | 5 -
> src/mesa/drivers/dri/i965/brw_state_cache.c | 12 +---
> 2 files changed, 5 insertions(+), 12 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_context.h
> b/src/mesa/drivers/dri/i965/brw_context.h
> index 0a29a69..82687bd 100644
> --- a/src/mesa/drivers/dri/i965/brw_context.h
> +++ b/src/mesa/drivers/dri/i965/brw_context.h
> @@ -854,8 +854,6 @@ struct brw_cache_item {
> };
>
>
> -typedef void (*cache_aux_free_func)(const void *aux);
> -
> struct brw_cache {
> struct brw_context *brw;
>
> @@ -865,9 +863,6 @@ struct brw_cache {
>
> uint32_t next_offset;
> bool bo_used_by_gpu;
> -
> - /** Optional functions for freeing other pointers attached to a
> prog_data. */
> - cache_aux_free_func aux_free[BRW_MAX_CACHE];
> };
>
>
> diff --git a/src/mesa/drivers/dri/i965/brw_state_cache.c
> b/src/mesa/drivers/dri/i965/brw_state_cache.c
> index 2fbcd14..7f07bef 100644
> --- a/src/mesa/drivers/dri/i965/brw_state_cache.c
> +++ b/src/mesa/drivers/dri/i965/brw_state_cache.c
> @@ -349,11 +349,6 @@ brw_init_caches(struct brw_context *brw)
> 4096, 64);
> if (brw->has_llc)
>drm_intel_gem_bo_map_unsynchronized(cache->bo);
> -
> - cache->aux_free[BRW_CACHE_VS_PROG] = brw_stage_prog_data_free;
> - cache->aux_free[BRW_CACHE_GS_PROG] = brw_stage_prog_data_free;
> - cache->aux_free[BRW_CACHE_FS_PROG] = brw_stage_prog_data_free;
> - cache->aux_free[BRW_CACHE_CS_PROG] = brw_stage_prog_data_free;
> }
>
> static void
> @@ -367,9 +362,12 @@ brw_clear_cache(struct brw_context *brw, struct
> brw_cache *cache)
> for (i = 0; i < cache->size; i++) {
>for (c = cache->items[i]; c; c = next) {
> next = c->next;
> - if (cache->aux_free[c->cache_id]) {
> + if (c->cache_id == BRW_CACHE_VS_PROG ||
> + c->cache_id == BRW_CACHE_GS_PROG ||
> + c->cache_id == BRW_CACHE_FS_PROG ||
> + c->cache_id == BRW_CACHE_CS_PROG) {
> const void *item_aux = c->key + c->key_size;
> -cache->aux_free[c->cache_id](item_aux);
> +brw_stage_prog_data_free(item_aux);
> }
> free((void *)c->key);
> free(c);
> --
> 2.5.0
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 1/2] i965: remove cache_aux_free_func array
There is only one function that can be called, which is well known at
compilation time.
The abstraction used here seems unnecessary, so let's use a direct call
to brw_stage_prog_data_free() when appropriate, cut down the size of
struct brw_cache.
Signed-off-by: Emil Velikov
---
src/mesa/drivers/dri/i965/brw_context.h | 5 -
src/mesa/drivers/dri/i965/brw_state_cache.c | 12 +---
2 files changed, 5 insertions(+), 12 deletions(-)
diff --git a/src/mesa/drivers/dri/i965/brw_context.h
b/src/mesa/drivers/dri/i965/brw_context.h
index 0a29a69..82687bd 100644
--- a/src/mesa/drivers/dri/i965/brw_context.h
+++ b/src/mesa/drivers/dri/i965/brw_context.h
@@ -854,8 +854,6 @@ struct brw_cache_item {
};
-typedef void (*cache_aux_free_func)(const void *aux);
-
struct brw_cache {
struct brw_context *brw;
@@ -865,9 +863,6 @@ struct brw_cache {
uint32_t next_offset;
bool bo_used_by_gpu;
-
- /** Optional functions for freeing other pointers attached to a prog_data.
*/
- cache_aux_free_func aux_free[BRW_MAX_CACHE];
};
diff --git a/src/mesa/drivers/dri/i965/brw_state_cache.c
b/src/mesa/drivers/dri/i965/brw_state_cache.c
index 2fbcd14..7f07bef 100644
--- a/src/mesa/drivers/dri/i965/brw_state_cache.c
+++ b/src/mesa/drivers/dri/i965/brw_state_cache.c
@@ -349,11 +349,6 @@ brw_init_caches(struct brw_context *brw)
4096, 64);
if (brw->has_llc)
drm_intel_gem_bo_map_unsynchronized(cache->bo);
-
- cache->aux_free[BRW_CACHE_VS_PROG] = brw_stage_prog_data_free;
- cache->aux_free[BRW_CACHE_GS_PROG] = brw_stage_prog_data_free;
- cache->aux_free[BRW_CACHE_FS_PROG] = brw_stage_prog_data_free;
- cache->aux_free[BRW_CACHE_CS_PROG] = brw_stage_prog_data_free;
}
static void
@@ -367,9 +362,12 @@ brw_clear_cache(struct brw_context *brw, struct brw_cache
*cache)
for (i = 0; i < cache->size; i++) {
for (c = cache->items[i]; c; c = next) {
next = c->next;
- if (cache->aux_free[c->cache_id]) {
+ if (c->cache_id == BRW_CACHE_VS_PROG ||
+ c->cache_id == BRW_CACHE_GS_PROG ||
+ c->cache_id == BRW_CACHE_FS_PROG ||
+ c->cache_id == BRW_CACHE_CS_PROG) {
const void *item_aux = c->key + c->key_size;
-cache->aux_free[c->cache_id](item_aux);
+brw_stage_prog_data_free(item_aux);
}
free((void *)c->key);
free(c);
--
2.5.0
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
