[Mesa-dev] [PATCH 02/10] glsl: serialize methods for IR instructions

[Paul Berry]

On 29 January 2014 01:24, Tapani Pälli <tapani.palli at 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 <stereotype441 at gmail.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20140204/d43e372c/attachment.html>