On 6 November 2013 00:59, Tapani Pälli <tapani.pa...@intel.com> wrote:
> On 11/06/2013 12:02 AM, Paul Berry wrote: > > On 1 November 2013 02:16, Tapani Pälli <tapani.pa...@intel.com> wrote: > >> +static void >> +calc_item(const void *key, void *data, void *closure) >> > > How about "increment_count" for the name of this function? > > > ok > > > > >> +{ >> + unsigned *sz = (unsigned *) closure; >> + *sz = *sz + 1; >> +} >> + >> + >> +static unsigned >> +_hash_table_size(struct string_to_uint_map *map) >> > > This function is global, so its name can't start with an > underscore--such names are reserved for library functions. Same goes for > many of the other functions in this file. > > > will change > > > > >> +{ >> + unsigned size = 0; >> + map->iterate(calc_item, &size); >> + return size; >> +} >> + >> + >> +static void >> +serialize_item(const void *key, void *data, void *closure) >> +{ >> + memory_writer *blob = (memory_writer *) closure; >> + uint32_t value = ((intptr_t)data); >> + >> + blob->write_string((char *)key); >> + blob->write_uint32(&value); >> +} >> + >> + >> +static void >> +_serialize_hash_table(struct string_to_uint_map *map, memory_writer >> *blob) >> +{ >> + uint32_t size = _hash_table_size(map); >> + blob->write_uint32(&size); >> + map->iterate(serialize_item, blob); >> > > Rather than take two passes over the hash table (one to compute its size > and one to output its contents), why not do the trick that we do with > overwrite() in ir_cache_serializer.cpp? (In other words, reserve space for > the size of the hashtable in bytes, then serialize the hashtable, then > overwrite the reserved space with the actual size). > > > > yes this is faster, will change > > > + /* Shaders IR, to be decided if we want these to be available */ >> > > Has there been previous discussion on the mesa-dev list about whether or > not we want these to be available? If so, please include a link to > previous discussion and a brief summary. if not, what does the decision > hinge on? At first blush it seems to me that there's no reason to > serialize the IR of the individual shaders, since ARB_get_program_binary > operates on full programs rather than individual shaders. > > > Nope there has been no previous discussion about this. Automatic cache > will need these to be able to quick exit from glCompileShader (if wanted), > it's easier for the implementation to have them as separate blobs than part > of whole program blob though. With the extension I'm not sure if unlinked > IR is required for possible run-time recompilations? I'd like someone to > confirm this. > Yeah, you're probably right that we need to store the individual shaders' IR for automatic caching. For OES_get_program_binary, based on my reading of the spec, we shouldn't store the individual shaders' IR. ProgramBinaryOES only loads the linked shader; it leaves the compiled shader objects (if any) alone. > > > > Also, typically we prefer to disable code using "if (false)" rather > than ifdefs, because that ensures that the disabled code still compiles. > (Otherwise, it's much more likely to bit rot as the rest of the code base > evolves). > > > ok, will use if (false), I also changethe unlinked shader define to use > your STORE_UNLINKED_SHADERS proposal > > > > Are we going to attempt to store the back-end compiled shaders? It > seems like we'd have to do that in order to get a lot of the benefit of > ARB_get_program_binary. > > > Yes it is definitely required, there is big amount of time spent in > Driver->LinkShader(). However, this was left out for now. It might be that > several back-end blobs are needed and I'm not sure how well it works and > what should happen when recompilation happens. This needs a bit more > thought. At least new API is needed with driver to simply retrieve and put > binary blobs, I would certainly like some advice how to approach the > problem. > > // Tapani > >
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev