On 29 January 2014 01:24, Tapani Pälli <tapani.pa...@intel.com> wrote:
> +
> +
> +/**
> + * Serialization of exec_list, writes length of the list +
> + * calls serialize_data for each instruction
> + */
> +void
> +serialize_list(exec_list *list, memory_writer &mem)
> +{
> + uint32_t list_len = 0;
> + foreach_list(node, list)
> + list_len++;
> +
> + mem.write_uint32_t(list_len);
> +
> + foreach_list(node, list)
> + ((ir_instruction *)node)->serialize(mem);
>
Nit pick: we could avoid having to walk the list twice by using
mem.overwrite() to write out the length once we've written all the list
elements. A similar comment applies to ir_call::serialize_data().
> +}
> +
> +
> +static void
> +serialize_glsl_type(const glsl_type *type, memory_writer &mem)
> +{
> + uint32_t data_len = 0;
> +
> + mem.write_string(type->name);
> +
> + unsigned start_pos = mem.position();
> + mem.write_uint32_t(data_len);
> +
> + uint32_t type_hash;
> +
> + /**
> + * notify reader if a user defined type exists already
> + * (has been serialized before)
> + */
> + uint8_t user_type_exists = 0;
> +
> + /* serialize only user defined types */
> + switch (type->base_type) {
> + case GLSL_TYPE_ARRAY:
> + case GLSL_TYPE_STRUCT:
> + case GLSL_TYPE_INTERFACE:
> + break;
> + default:
> + goto glsl_type_serilization_epilogue;
> + }
> +
>
With the changes I recommended in the last patch, I believe we can replace
everything from here...
> + type_hash = _mesa_hash_data(type, sizeof(glsl_type));
>
+
> + /* check if this type has been written before */
> + if (mem.data_was_written((void *)type, type_hash))
> + user_type_exists = 1;
> + else
> + mem.mark_data_written((void *)type, type_hash);
>
...to here with:
user_type_exists = mem.make_unique_id(type, &type_hash);
Although I'd recommend renaming type_hash to type_id so that there's no
confusion about the fact that it's a unique ID and not a hash (hashes
aren't guaranteed to be unique).
> +
> + mem.write_uint8_t(user_type_exists);
> + mem.write_uint32_t(type_hash);
> +
> + /* no need to write again ... */
> + if (user_type_exists)
> + goto glsl_type_serilization_epilogue;
>
Nit pick: how about instead of the goto, just do:
if (!user_type_exists) {
/* glsl type data */
mem.write_uint32_t((uint32_t) type->length);
...
}
data_len = mem.position() - start_pos - sizeof(data_len);
...
> +
> + /* glsl type data */
> + mem.write_uint32_t((uint32_t)type->length);
> + mem.write_uint8_t((uint8_t)type->base_type);
> + mem.write_uint8_t((uint8_t)type->interface_packing);
> +
> + if (type->base_type == GLSL_TYPE_ARRAY)
> + serialize_glsl_type(type->element_type(), mem);
> + else if (type->base_type == GLSL_TYPE_STRUCT ||
> + type->base_type == GLSL_TYPE_INTERFACE) {
> + glsl_struct_field *field = type->fields.structure;
> + for (unsigned k = 0; k < type->length; k++, field++) {
> + mem.write_string(field->name);
> + serialize_glsl_type(field->type, mem);
> + mem.write_uint8_t((uint8_t)field->row_major);
> + mem.write_int32_t(field->location);
> + mem.write_uint8_t((uint8_t)field->interpolation);
> + mem.write_uint8_t((uint8_t)field->centroid);
> + }
> + }
> +
> +glsl_type_serilization_epilogue:
> +
> + data_len = mem.position() - start_pos - sizeof(data_len);
> + mem.overwrite(&data_len, sizeof(data_len), start_pos);
> +}
> +
> +
> +void
> +ir_variable::serialize_data(memory_writer &mem)
> +{
> + /* name can be NULL, see ir_print_visitor for explanation */
> + const char *non_null_name = name ? name : "parameter";
>
Since mem.write_string handles NULL strings, can we get rid of this?
> + int64_t unique_id = (int64_t) (intptr_t) this;
> + uint8_t mode = data.mode;
> + uint8_t has_constant_value = constant_value ? 1 : 0;
> + uint8_t has_constant_initializer = constant_initializer ? 1 : 0;
> +
> + serialize_glsl_type(type, mem);
> +
> + mem.write_string(non_null_name);
> + mem.write_int64_t(unique_id);
> + mem.write_uint8_t(mode);
> +
> + mem.write(&data, sizeof(data));
> +
> + mem.write_uint32_t(num_state_slots);
> + mem.write_uint8_t(has_constant_value);
> + mem.write_uint8_t(has_constant_initializer);
> +
> + for (unsigned i = 0; i < num_state_slots; i++) {
> + mem.write_int32_t(state_slots[i].swizzle);
> + for (unsigned j = 0; j < 5; j++) {
> + mem.write_int32_t(state_slots[i].tokens[j]);
> + }
> + }
> +
> + if (constant_value)
> + constant_value->serialize(mem);
> +
> + if (constant_initializer)
> + constant_initializer->serialize(mem);
> +
> + uint8_t has_interface_type = get_interface_type() ? 1 : 0;
> +
> + mem.write_uint8_t(has_interface_type);
> + if (has_interface_type)
> + serialize_glsl_type(get_interface_type(), mem);
> +}
>
Everything's a nit pick except for the suggestion to use make_unique_id().
With that fixed, this patch is:
Reviewed-by: Paul Berry <stereotype...@gmail.com>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
