[Mesa-dev] [v2 3/6] glsl: ir_serializer class for the shader cache

[Paul Berry]

On 5 November 2013 23:45, Tapani Pälli <tapani.palli at intel.com> wrote:

>  On 11/05/2013 07:36 PM, Paul Berry wrote:
>
> On 1 November 2013 02:16, Tapani Pälli <tapani.palli at intel.com> wrote:
>
>>
>> +
>> +/**
>> + * Function to create an unique string for a ir_variable. This is
>> + * used by variable dereferences to indicate the exact ir_variable
>> + * when deserialization happens.
>>
>
>  I still don't understand why we don't just use the pointer for this
> purpose.  It's unique, and it takes up much less storage than
> <name>_<decimal address>.
>
>
>
> We need to construct a unique name to refer to when reading so this is
> constructed here already. It could be also just a counter like is used
> somewhere else (to have assignment_tmp at 1, assignment_tmp at 2) but I wanted
> to keep this code minimal as it's just unique naming.
>

I'm sorry, there's still something I'm not understanding.  Before
serialization, the names are not unique.  So I don't see why we need to
create unique names when reading.  The goal should be to reproduce the IR
data structure that was saved as closely as possible.

It seems like storing the address of the ir_variable (or some other
suitably determined unique numeric id) should be sufficient for this.  When
we serialize the ir_variable, we write out its unique numeric id.  When we
serialize an ir_dereference_variable, we write out the unique numeric id of
the ir_variable it refers to.  When deserializing an ir_variable, we store
the mapping from its unique numeric id to its new address in memory in a
hash table.  When deserializing an ir_dereference_variable, we look up the
unique numeric id to find the address of the ir_variable that's already
been deserialized.

It doesn't seem to me that uniqueness of names is necessary anywhere in
that process.  What am I missing?


>
>    +
>> +   for (unsigned i = 0; i < ir->num_state_slots; i++) {
>> +      blob.write_int32(&ir->state_slots[i].swizzle);
>>
>
>  Swizzles are unsigned 8-bit values.  This should be write_uint8.
>
>
>
> OK, maybe the struct could also be changed to have only 8 bits instead of
> a int then.
>

I could get behind that change.


>
>
>   +   CACHE_DEBUG("write %d prototypes\n", total);
>
>> +
>> +   foreach_list_const(node, shader->ir) {
>> +      ir_instruction *const inst = (ir_instruction *) node;
>> +      if (inst->as_variable())
>> +         if (save(inst))
>> +            goto write_errors;
>> +   }
>> +
>> +   foreach_list_const(node, shader->ir) {
>> +      ir_instruction *const inst = (ir_instruction *) node;
>> +      if (inst->as_function())
>> +         if (save(inst))
>> +            goto write_errors;
>> +   }
>>
>
>  Why is it necessary to save the variables and instructions first?
>
>
> This is because during parsing we might encounter a call to a function
> that does not exist yet, same goes for variable references. Another way
> would be to modify the reading side so that it makes 2 passes over the data
> but I took this way as originally reader did not use mmap so it was just
> simpler.
>

Ok, I think you are correct about the functions.  But I believe for
variables, the ir_variable always appears in the IR before any references
to it.  Can someone confirm this? (Ken or Ian perhaps?)
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20131106/df3f67c5/attachment.html>