<html>
<head>
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<div class="moz-cite-prefix">On 11/05/2013 07:36 PM, Paul Berry
wrote:<br>
</div>
<blockquote
cite="mid:CA+yLL65zSsQ1ZGh8yG8Op4OJ4-5Rm6f4vYFu1BzUbv5qPv0LdQ@mail.gmail.com"
type="cite">
<meta http-equiv="Content-Type" content="text/html;
charset=ISO-8859-1">
<div dir="ltr">On 1 November 2013 02:16, Tapani Pälli <span
dir="ltr"><<a moz-do-not-send="true"
href="mailto:tapani.palli@intel.com" target="_blank">tapani.palli@intel.com</a>></span>
wrote:<br>
<div class="gmail_extra">
<div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">ir_serializer
can serialize a gl_shader structure as binary<br>
data, will be used by OES_get_program_binary
implementation.<br>
+ uint8_t sampler_array = type->sampler_array;<br>
+ uint8_t sampler_type = type->sampler_type;<br>
+ uint8_t interface_packing =
type->interface_packing;<br>
+<br>
+ blob.write_uint32(&base_type);<br>
</blockquote>
<div><br>
</div>
<div>There's no need for base_type to be 32 bits. It's a
tiny enum.<br>
</div>
<div> </div>
</div>
</div>
</div>
</blockquote>
<br>
Sure, will change.<br>
<br>
<blockquote
cite="mid:CA+yLL65zSsQ1ZGh8yG8Op4OJ4-5Rm6f4vYFu1BzUbv5qPv0LdQ@mail.gmail.com"
type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">
+ blob.write_uint32(&length);<br>
+ blob.write_uint8(&vector_elms);<br>
+ blob.write_uint8(&matrix_cols);<br>
+ blob.write_uint8(&sampler_dimensionality);<br>
+ blob.write_uint8(&sampler_shadow);<br>
+ blob.write_uint8(&sampler_array);<br>
+ blob.write_uint8(&sampler_type);<br>
+ blob.write_uint8(&interface_packing);<br>
</blockquote>
<div><br>
</div>
<div>Two comments about this:<br>
<br>
1. Having write_uint8 and write_uint32 take their
arguments as pointers is surprising, and it makes code
like the above needlessly verbose.<br>
<br>
I'd recommend changing write_uint8 and write_uint32 take
their arguments as values; then we can just write:<br>
<br>
</div>
<div>blob.write_uint32(type->base_type);<br>
</div>
<div>blob.write_uint32(type->length);<br>
</div>
<div>blob.write_uint8(vector_elms);<br>
</div>
<div>...and so on.<br>
</div>
</div>
</div>
</div>
</blockquote>
<br>
Yes, can be changed. Using a pointer is kind of a leftover from
'make it look like fwrite' obsession I had.<br>
<br>
<blockquote
cite="mid:CA+yLL65zSsQ1ZGh8yG8Op4OJ4-5Rm6f4vYFu1BzUbv5qPv0LdQ@mail.gmail.com"
type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<div>2. I'm not sure it makes sense to serialize all of
these fields of glsl_type. Really there are only 4 kinds
of GLSL types: built-in types, structs, interfaces, and
arrays. Most of the fields above only matter for built-in
types. And for built-in types, we don't want to serialize
the entire glsl_type structure; we simply want to record
which built-in type is being referred to so that when the
shader is loaded later, we can look up one of the existing
flyweights.<br>
<br>
</div>
</div>
</div>
</div>
</blockquote>
<br>
There are user defined structures and arrays or arrays of user
defined structures, for reconstructing those most (if not all) of
this data is needed. For built-in types I agree, that could be
optimized to be much smaller and faster, I'm not sure if there is a
way to detect if the type is a builtin type currently but it could
be added.<br>
<br>
<blockquote
cite="mid:CA+yLL65zSsQ1ZGh8yG8Op4OJ4-5Rm6f4vYFu1BzUbv5qPv0LdQ@mail.gmail.com"
type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<div>I have additional thoughts further below about saving
GLSL types that would make it unnecessary to save built-in
types at all.<br>
</div>
<div> </div>
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">
+<br>
+ if (type->base_type == GLSL_TYPE_ARRAY)<br>
+ save_glsl_type(blob, type->element_type());<br>
+ else if (type->base_type == GLSL_TYPE_STRUCT ||<br>
+ type->base_type == GLSL_TYPE_INTERFACE) {<br>
+ glsl_struct_field *field =
type->fields.structure;<br>
+ for (unsigned k = 0; k < length; k++, field++) {<br>
+ blob.write_string(field->name);<br>
+ save_glsl_type(blob, field->type);<br>
+ uint8_t row_major = field->row_major;<br>
+ uint8_t interpolation = field->interpolation;<br>
+ uint8_t centroid = field->centroid;<br>
+ blob.write_uint8(&row_major);<br>
+ blob.write_int32(&field->location);<br>
+ blob.write_uint8(&interpolation);<br>
+ blob.write_uint8(¢roid);<br>
</blockquote>
<div><br>
</div>
<div>I'd like to make one more argument in favor of having
the serialization and deserialization functions in the
classes themselves. If we did so, then we could have a
glsl_type::serialize() function as well as a
glsl_struct_field::serialize() function. Then the code
above would collapse into simply:<br>
<br>
</div>
<div>else if(type->base_type == GLSL_TYPE_STRUCT ||<br>
</div>
<div> type->base_type == GLSL_TYPE_INTERFACE) {<br>
</div>
<div> for (unsigned k = 0; k < length; k++)<br>
</div>
<div> type->fields.structure[k].serialize(blob);<br>
}<br>
</div>
<div> </div>
</div>
</div>
</div>
</blockquote>
<br>
Yes, I think I have bought this idea already. I'll refactor the code
as methods of IR but then it would be nice also to modify IR classes
themselves (which I was asked not to do when starting the work ...).
For example group variables and bitfields as a nice data section of
its own that can be dumped at one go without cache needing to know
about the exact types of these variables.<br>
<br>
<blockquote
cite="mid:CA+yLL65zSsQ1ZGh8yG8Op4OJ4-5Rm6f4vYFu1BzUbv5qPv0LdQ@mail.gmail.com"
type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">
+ }<br>
+ }<br>
+<br>
+ ir_len = blob.position() - start_pos - sizeof(ir_len);<br>
+ blob.overwrite(&ir_len, sizeof(ir_len),
start_pos);<br>
+ return 0;<br>
+}<br>
+<br>
+<br>
+/**<br>
+ * Function to create an unique string for a ir_variable.
This is<br>
+ * used by variable dereferences to indicate the exact
ir_variable<br>
+ * when deserialization happens.<br>
</blockquote>
<div><br>
</div>
<div>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>.<br>
</div>
<div> </div>
</div>
</div>
</div>
</blockquote>
<br>
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@1, assignment_tmp@2) but
I wanted to keep this code minimal as it's just unique naming.<br>
<br>
<br>
<blockquote
cite="mid:CA+yLL65zSsQ1ZGh8yG8Op4OJ4-5Rm6f4vYFu1BzUbv5qPv0LdQ@mail.gmail.com"
type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">
+ */<br>
+static char *_unique_name(ir_variable *var)<br>
+{<br>
+ return ralloc_asprintf(NULL,<br>
+ "%s_%ld", var->name ? var->name :
"parameter", (intptr_t) var);<br>
+}<br>
+<br>
+<br>
+int<br>
+ir_serializer::save_ir(ir_variable *ir)<br>
+{<br>
+ /* name can be NULL, see ir_print_visitor for
explanation */<br>
+ const char *non_null_name = ir->name ? ir->name
: "parameter";<br>
+ char *uniq = _unique_name(ir);<br>
+<br>
+ save_glsl_type(blob, ir->type);<br>
</blockquote>
<div><br>
</div>
<div>It seems really wasteful to have the save_ir()
functions for ir_variable, ir_constant, ir_expression,
ir_function_signature, and ir_texture all call
save_glsl_type(). The fact is that in any given GLSL
program, there is a small set of types that are used over
and over again, and they are mostly built-in types.
Calling save_glsl_type() everywhere a type appears in the
IR is going to cause an enormous amount of bloat in the
file size.<br>
<br>
Here's an alternative idea:<br>
<br>
</div>
<div>1. While serializing, keep track of a hashable that
maps each glsl_type to a small integer. Instead of
storing the entire type when we're serializing, just store
the small integers. While serializing, if we encounter a
type that's not in the hash table, assign it to the next
available small integer.<br>
<br>
</div>
<div>2. Preload this hashtable with a fixed static mapping
for built-in types (e.g. 0=float, 1=vec2, 2=vec3, 3=vec4,
4=int, and so on).<br>
<br>
</div>
<div>3. After serializing the IR for the shader, go through
the hashtable and serialize all of the non-built-in types
with their associated integers.<br>
<br>
</div>
<div>4. When deserializing, read the hashtable mapping
integers to types first. Then, while deserializing the
IR, each integer can just be looked up in the hashtable to
find the associated glsl_type.<br>
<br>
</div>
<div>This should cut dramatically down on storage size,
which should make the loading process much faster. It
also avoids the need to store the built-in types at all.<br>
</div>
<div> </div>
</div>
</div>
</div>
</blockquote>
<br>
This sounds good to me, this hash could be part of the 'prototypes'
section.<br>
<br>
8<<br>
<br>
<blockquote
cite="mid:CA+yLL65zSsQ1ZGh8yG8Op4OJ4-5Rm6f4vYFu1BzUbv5qPv0LdQ@mail.gmail.com"
type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">
+ blob.write_uint8(&has_constant_value);<br>
+ blob.write_uint8(&has_constant_initializer);<br>
</blockquote>
<div><br>
</div>
<div>It seems backwards that in the in-memory representation
(the ir_variable class) we pack things like read_only,
centroid, invariant, etc. tightly into bitfields, but we
serialize it out to a byte-aligned data structure.
Usually people try to make the serialized data format more
tightly packed than the in-memory format, because the
tradeoffs for serialized data weigh more strongly in favor
of packing (access speed is less important; small
footprint is more important).<br>
<br>
I think we should consider ways to maintain the tight
bitfield packing in the serialized data format. Here's
one idea: how about if we refactor ir_variable so that it
contains a substructure called "bits", which contains all
the fields that are simple integers, packed carefully
together. Then, here in the serialization function, we
just write out the "bits" data structure verbatim using a
single call to memory_writer::write().<br>
<br>
In addition to making the file size smaller and the
serialization/deserialization code simpler and faster,
this would have the advantage that if we add new fields to
ir_variable::bits in the future, we won't have to go to
any effort to ensure that they will be serialized
correctly. The right thing will just happen
automatically.<br>
</div>
<div> </div>
</div>
</div>
</div>
</blockquote>
<br>
I'm all for doing this and this is what I was proposing when
discussing the cache implementation some months ago with Ian and
Kenneth. Back then the discussion was to not modify IR contents but
rather have a working implementation first and then optimize later,
but maybe I've reached the point that now :) Ian, could you comment
on this? Having a separate data structure within each instruction
would allow also to use some helper data serialization libraries if
wanted.<br>
<br>
<blockquote
cite="mid:CA+yLL65zSsQ1ZGh8yG8Op4OJ4-5Rm6f4vYFu1BzUbv5qPv0LdQ@mail.gmail.com"
type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">
+<br>
+ for (unsigned i = 0; i < ir->num_state_slots;
i++) {<br>
+
blob.write_int32(&ir->state_slots[i].swizzle);<br>
</blockquote>
<div><br>
</div>
<div>Swizzles are unsigned 8-bit values. This should be
write_uint8.<br>
</div>
<div> </div>
</div>
</div>
</div>
</blockquote>
<br>
OK, maybe the struct could also be changed to have only 8 bits
instead of a int then.<br>
<br>
<blockquote
cite="mid:CA+yLL65zSsQ1ZGh8yG8Op4OJ4-5Rm6f4vYFu1BzUbv5qPv0LdQ@mail.gmail.com"
type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">
+ for (unsigned j = 0; j < 5; j++) {<br>
+
blob.write_int32(&ir->state_slots[i].tokens[j]);<br>
</blockquote>
<div><br>
</div>
<div>Tokens are always either small integers or references
to the gl_state_index_ enum. So these can be stored as
uint8's. (It might be worth adding an assertion just to
be on the safe side though).<br>
</div>
</div>
</div>
</div>
</blockquote>
<br>
This is of type int in the state_slots structure also.<br>
<br>
<blockquote
cite="mid:CA+yLL65zSsQ1ZGh8yG8Op4OJ4-5Rm6f4vYFu1BzUbv5qPv0LdQ@mail.gmail.com"
type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<div> </div>
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">
+ }<br>
+ }<br>
+<br>
+ CACHE_DEBUG("save ir_variable [%s] [%s]\n",
non_null_name, uniq);<br>
+<br>
+ ralloc_free(uniq);<br>
+<br>
+ if (ir->constant_value)<br>
+ if (save(ir->constant_value))<br>
+ return -1;<br>
+<br>
+ if (ir->constant_initializer)<br>
+ if (save(ir->constant_initializer))<br>
+ return -1;<br>
+<br>
+ uint8_t has_interface_type =<br>
+ ir->is_interface_instance() ? 1 : 0;<br>
+<br>
+ blob.write_uint8(&has_interface_type);<br>
+<br>
+ if (has_interface_type)<br>
+ save_glsl_type(blob, ir->get_interface_type());<br>
</blockquote>
<div><br>
</div>
<div>We talked about this some previously, but I really
think the convention of having these serialization
functions return "int" (with 0 indicating success) is
going to cause maintenance problems for us. Here's my
reasoning:<br>
<br>
</div>
<div>1. Since 0 means false we have to do mental gymnastics
every time we see something like:<br>
<br>
</div>
<div>if (save(ir->constant_value))<br>
</div>
<div> return -1;<br>
<br>
</div>
<div>to remind ourselves that the branch is taken on
failure, not success.<br>
<br>
</div>
<div>2. If we really are trying to plan for a future where
these ints can be return codes instead of success/failure
indicators, then doing this:<br>
<br>
if (save(ir->constant_value))<br>
</div>
<div> return -1;<br>
<br>
</div>
<div>won't work anyway. We'd have to do something like
this:<br>
<br>
</div>
<div>int retval = save(ir->constant_value));<br>
</div>
<div>if (retval)<br>
</div>
<div> return retval;<br>
<br>
</div>
<div>3. It's going to be very hard to remember to check the
return values on these functions. You yourself missed
checking the return value here:<br>
<br>
</div>
<div>if (has_interface_type)<br>
</div>
<div> save_glsl_type(blob, ir->get_interface_type());<br>
<br>
</div>
<div>4. All of this is speculative future-proofing anyway.
The only places in ir_cache_serializer that I can find
that would actually generate a nonzero return code are:
(a) if we find an unrecognized ir_type (would be better
handled by an assertion, or a pure virtual function, as I
note below) and (b) if memory allocation fails in
memory_writer::grow() (which I thought we agreed to
remove).<br>
<br>
</div>
<div>I recommend ripping out all of these integer return
types and returning void. If, in the future, there are
any legitimate error conditions that we want to track, we
should add an error state to memory_writer (or possibly
ir_serializer) and then check it at the end of
serialization. That's the technique we use for handling
parsing and compilation errors elsewhere in Mesa, and it's
served us very well.<br>
</div>
<div> </div>
</div>
</div>
</div>
</blockquote>
<br>
Removing these is sort of ok for me at this point, what it does is
that it moves these possible serialization errors to happen when
binary is read during deserialization. Good thing with having these
-1's here is that it will catch of the failure early so we never
will even attempt to parse this. This was quite useful during
development as I was simply leaving out shaders that cache could not
handle and this could be done in a very fine-grained way. As
imaginary example some shader with tricky expression was left out
but all rest other shaders were succesfully cached. I'm not sure if
we should try to make cache 100% proof or allow it to bypass those
shaders that are not simply supported by the cache yet, for example
because of some new added feature?<br>
<br>
<blockquote
cite="mid:CA+yLL65zSsQ1ZGh8yG8Op4OJ4-5Rm6f4vYFu1BzUbv5qPv0LdQ@mail.gmail.com"
type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">
+int<br>
+ir_serializer::save_ir(ir_expression *ir)<br>
+{<br>
+ int32_t num_operands = ir->get_num_operands();<br>
+ uint32_t exp_operation = ir->operation;<br>
+<br>
+ save_glsl_type(blob, ir->type);<br>
</blockquote>
<div><br>
</div>
<div>Is it necessary to save the glsl_type for ir_expression
nodes? ir_expression has constructors which can infer the
type based on the type of the arguments.<br>
</div>
<div> </div>
</div>
</div>
</div>
</blockquote>
<br>
I've thought so far it is, it is the type that results from the
expression but I can change it to use such constructor also.<br>
<br>
<blockquote
cite="mid:CA+yLL65zSsQ1ZGh8yG8Op4OJ4-5Rm6f4vYFu1BzUbv5qPv0LdQ@mail.gmail.com"
type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">
+<br>
+ blob.write_uint32(&exp_operation);<br>
+ blob.write_int32(&num_operands);<br>
+<br>
+ /* operand ir_type below is written to make parsing
easier */<br>
+ for (unsigned i = 0; i < ir->get_num_operands();
i++) {<br>
+ uint32_t ir_type = ir->operands[i]->ir_type;<br>
+ blob.write_uint32(&ir_type);<br>
+ if (save(ir->operands[i]))<br>
+ return -1;<br>
+ }<br>
+ return 0;<br>
+}<br>
+<br>
+<br>
+int<br>
+ir_serializer::save_ir(ir_function *ir)<br>
+{<br>
+ uint32_t sig_amount = 0;<br>
</blockquote>
<div><br>
</div>
<div>Can we rename this to "num_signatures"? "amount"
usualy implies a quantity that you don't measure by
counting.<br>
</div>
<div> </div>
</div>
</div>
</div>
</blockquote>
<br>
ok<br>
<br>
<blockquote
cite="mid:CA+yLL65zSsQ1ZGh8yG8Op4OJ4-5Rm6f4vYFu1BzUbv5qPv0LdQ@mail.gmail.com"
type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">
+ir_serializer::save_ir(ir_function_signature *ir)<br>
+{<br>
+ int32_t par_count = 0;<br>
</blockquote>
<div><br>
</div>
<div>Can we rename this to something like "param_count"?
It's not obvious what "par" is an abbreviation for.<br>
</div>
</div>
</div>
</div>
</blockquote>
<br>
ok<br>
<br>
<blockquote
cite="mid:CA+yLL65zSsQ1ZGh8yG8Op4OJ4-5Rm6f4vYFu1BzUbv5qPv0LdQ@mail.gmail.com"
type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">
+ if (prototypes_only)<br>
+ return 0;<br>
</blockquote>
<div><br>
</div>
<div>I'm a little concerned about this. If we ever make a
mistake where prototypes_only doesn't match up properly
between the serializer and the deserializer, the
deserializer will get hopelessly lost.<br>
<br>
</div>
<div>We could easily fix this by outputting a body_size of 0
whenever prototypes_only is true.<br>
</div>
<div> </div>
</div>
</div>
</div>
</blockquote>
<br>
I don't think such mismatch would happen but I can change this.<br>
<br>
<blockquote
cite="mid:CA+yLL65zSsQ1ZGh8yG8Op4OJ4-5Rm6f4vYFu1BzUbv5qPv0LdQ@mail.gmail.com"
type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">
+<br>
+ /* function body */<br>
+ foreach_iter(exec_list_iterator, iter, ir->body) {<br>
+ ir_instruction *const inst = (ir_instruction *)
iter.get();<br>
+ CACHE_DEBUG(" body instruction node type %d\n",
inst->ir_type);<br>
+ if (save(inst))<br>
+ return -1;<br>
+ }<br>
+<br>
+ return 0;<br>
+}<br>
+<br>
+<br>
+int<br>
+ir_serializer::save_ir(ir_assignment *ir)<br>
+{<br>
+ uint32_t write_mask = ir->write_mask;<br>
+<br>
+ blob.write_uint32(&write_mask);<br>
+<br>
+ /* lhs (ir_deference_*) */<br>
+ uint32_t lhs_type = ir->lhs->ir_type;<br>
+ blob.write_uint32(&lhs_type);<br>
</blockquote>
<div><br>
</div>
<div>Isn't this redundant? The call to save(ir->lhs)
just below will resolve to
ir_serializer::save(ir_instruction *), which writes out
the ir_type as its first action.<br>
</div>
<div> </div>
</div>
</div>
</div>
</blockquote>
<br>
These 'extra ir_type writes' are done to make parsing happen so that
each instruction knows what type it's going to get and can call
different function for parsing. I've added a 'read_rvalue' though to
I think most of these cases could be removed now. I will investigate
if there are still cases where the type is required to be known by
some instruction.<br>
<br>
<blockquote
cite="mid:CA+yLL65zSsQ1ZGh8yG8Op4OJ4-5Rm6f4vYFu1BzUbv5qPv0LdQ@mail.gmail.com"
type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">
+<br>
+ if (save(ir->lhs))<br>
+ return -1;<br>
+<br>
+ if (ir->condition) {<br>
+ CACHE_DEBUG("%s: assignment has condition, not
supported", __func__);<br>
</blockquote>
<div><br>
</div>
<div>Why don't we support this?<br>
</div>
<div> </div>
</div>
</div>
</div>
</blockquote>
<br>
I believe this just never triggered for me, maybe because of
optimizations took the conditions away. Support can be added and
verified when I have such a shader.<br>
<br>
<blockquote
cite="mid:CA+yLL65zSsQ1ZGh8yG8Op4OJ4-5Rm6f4vYFu1BzUbv5qPv0LdQ@mail.gmail.com"
type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">
+ir_serializer::save_ir(ir_swizzle *ir)<br>
+{<br>
+ uint32_t components = ir->mask.num_components;<br>
+ const uint32_t mask[4] = {<br>
+ ir->mask.x,<br>
+ ir->mask.y,<br>
+ ir->mask.z,<br>
+ ir->mask.w<br>
+ };<br>
+ uint32_t val_type = ir->val->ir_type;<br>
+<br>
+ blob.write_uint32(&components);<br>
+ blob.write(&mask, 4 * sizeof(mask[0]));<br>
</blockquote>
<div><br>
</div>
<div>Why not just serialize the ir_swizzle_mask data
structure in raw form? It will take up less space and be
faster.<br>
</div>
<div> </div>
</div>
</div>
</div>
</blockquote>
<br>
Sure, can be changed. I can see now that the structure itself has
also changed since this code was written.<br>
<br>
<blockquote
cite="mid:CA+yLL65zSsQ1ZGh8yG8Op4OJ4-5Rm6f4vYFu1BzUbv5qPv0LdQ@mail.gmail.com"
type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">
+ blob.write_int32(&op);<br>
+ blob.write_uint8(&has_coordinate);<br>
+ blob.write_uint8(&has_projector);<br>
+ blob.write_uint8(&has_shadow_comp);<br>
+ blob.write_uint8(&has_offset);<br>
</blockquote>
<div><br>
</div>
<div>This would be another good candidate for creating a
"bits" substructure, as I recommended with ir_variable.<br>
</div>
</div>
</div>
</div>
</blockquote>
<br>
Agreed.<br>
<br>
<blockquote
cite="mid:CA+yLL65zSsQ1ZGh8yG8Op4OJ4-5Rm6f4vYFu1BzUbv5qPv0LdQ@mail.gmail.com"
type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">
+ blob.write_uint8(&has_dpdy);<br>
</blockquote>
<div><br>
</div>
<div>There are a number of places in the serializer where we
have to go to extra effort to serialize out a 1 or 0 to
indicate whether an rvalue is present, and then only
serialize the rvalue if it is indeed present.<br>
<br>
How about if instead we modify
ir_serializer::save(ir_instruction *) so that if it's
passed a NULL pointer, it simply serializes out a magic
ir_type value that means "NULL"? (We could use
ir_type_unset, which is otherwise unused by the IR). This
would reduce our risk of bugs because there would be no
danger of forgetting to include this logic in a place
where an ir_rvalue is allowed to be NULL.<br>
</div>
</div>
</div>
</div>
</blockquote>
<br>
I'm ok with this proposal. I've had this change in my mind too but I
was a bit afraid it would make error situation detection header for
the parser code. I will think this through, I agree it would make
the code much more readable also.<br>
<br>
<blockquote
cite="mid:CA+yLL65zSsQ1ZGh8yG8Op4OJ4-5Rm6f4vYFu1BzUbv5qPv0LdQ@mail.gmail.com"
type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">
+<br>
+ foreach_iter(exec_list_iterator, iter,
ir->else_instructions) {<br>
+ ir_instruction *const inst = (ir_instruction *)
iter.get();<br>
+ CACHE_DEBUG(" ir_if else instruction node type
%d\n", inst->ir_type);<br>
+ if (save(inst))<br>
+ return -1;<br>
+ }<br>
</blockquote>
<div><br>
</div>
<div>I've seen a few places where this loop is repeated.
How about if we make a function whose job is to serialize
an exec_list of ir_instructions (including serializing its
length)?<br>
</div>
</div>
</div>
</div>
</blockquote>
<br>
Agreed, good idea.<br>
<br>
<br>
<blockquote
cite="mid:CA+yLL65zSsQ1ZGh8yG8Op4OJ4-5Rm6f4vYFu1BzUbv5qPv0LdQ@mail.gmail.com"
type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">
+<br>
+int ir_serializer::save_ir(ir_loop_jump *ir)<br>
+{<br>
+ uint32_t mode = ir->mode;<br>
+ return blob.write_uint32(&mode);<br>
</blockquote>
<div><br>
</div>
<div>mode is a tiny enum and should be serialized as a
uint8.<br>
</div>
</div>
</div>
</div>
</blockquote>
<br>
will fix<br>
<br>
<blockquote
cite="mid:CA+yLL65zSsQ1ZGh8yG8Op4OJ4-5Rm6f4vYFu1BzUbv5qPv0LdQ@mail.gmail.com"
type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">
+ blob.write_uint32(&ir_type);<br>
</blockquote>
<div><br>
</div>
<div>ir_type is a small enum and should be serialized as a
uint8.<br>
</div>
</div>
</div>
</div>
</blockquote>
<br>
will fix<br>
<br>
<br>
<blockquote
cite="mid:CA+yLL65zSsQ1ZGh8yG8Op4OJ4-5Rm6f4vYFu1BzUbv5qPv0LdQ@mail.gmail.com"
type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">
+ case ir_type_emit_vertex:<br>
+ SAVE_IR(ir_emit_vertex);<br>
+ break;<br>
+ case ir_type_end_primitive:<br>
+ SAVE_IR(ir_end_primitive);<br>
+ break;<br>
+<br>
+ default:<br>
+ CACHE_DEBUG("%s: error, type %d not implemented\n",<br>
+ __func__, ir->ir_type);<br>
+ return -1;<br>
+ }<br>
</blockquote>
<div><br>
</div>
<div>Another advantage of moving the serialization logic
into the IR classes is that we could do this dispatch by a
pure virtual function rather than a switch statement.
Then there would be no need to have a default case to do
error handling if we encounter an unexpected ir_type.
Instead, the compiler would check that each type of IR had
a corresponding implementation of the pure virtual.<br>
</div>
<div> </div>
</div>
</div>
</div>
</blockquote>
<br>
True, there is no need for this then.<br>
<br>
<blockquote
cite="mid:CA+yLL65zSsQ1ZGh8yG8Op4OJ4-5Rm6f4vYFu1BzUbv5qPv0LdQ@mail.gmail.com"
type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">
+_dump_bool(bool value, memory_writer &blob)<br>
+{<br>
+ uint8_t val = value;<br>
+ blob.write_uint8(&val);<br>
+}<br>
</blockquote>
<div><br>
</div>
<div>This seems like it should be in the memory_writer class
as<br>
<br>
void memory_writer::write_bool(bool value);<br>
</div>
</div>
</div>
</div>
</blockquote>
<br>
True, I will add this there.<br>
<br>
<blockquote
cite="mid:CA+yLL65zSsQ1ZGh8yG8Op4OJ4-5Rm6f4vYFu1BzUbv5qPv0LdQ@mail.gmail.com"
type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">
+ _dump_bool(state->ARB_gpu_shader5_warn, blob);<br>
+ _dump_bool(state->AMD_vertex_shader_layer_enable,
blob);<br>
+ _dump_bool(state->AMD_vertex_shader_layer_warn,
blob);<br>
+
_dump_bool(state->ARB_shading_language_420pack_enable,
blob);<br>
+
_dump_bool(state->ARB_shading_language_420pack_warn,
blob);<br>
+ _dump_bool(state->EXT_shader_integer_mix_enable,
blob);<br>
+ _dump_bool(state->EXT_shader_integer_mix_warn,
blob);<br>
</blockquote>
<div><br>
</div>
<div>This is another case where I'm concerned about us
forgetting to update this code when we add new extension
flags. I think this would be a good candidate for the
"bits" idea that I talked about above with ir_variable.
Although in this case I might call the structure something
like "enables" rather than "bits".<br>
</div>
</div>
</div>
</div>
</blockquote>
<br>
Yes, I'll make a separate patch to get them as a structure that is
easy to serialize.<br>
<br>
<blockquote
cite="mid:CA+yLL65zSsQ1ZGh8yG8Op4OJ4-5Rm6f4vYFu1BzUbv5qPv0LdQ@mail.gmail.com"
type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">+ CACHE_DEBUG("write %d
prototypes\n", total);<br>
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">
+<br>
+ foreach_list_const(node, shader->ir) {<br>
+ ir_instruction *const inst = (ir_instruction *)
node;<br>
+ if (inst->as_variable())<br>
+ if (save(inst))<br>
+ goto write_errors;<br>
+ }<br>
+<br>
+ foreach_list_const(node, shader->ir) {<br>
+ ir_instruction *const inst = (ir_instruction *)
node;<br>
+ if (inst->as_function())<br>
+ if (save(inst))<br>
+ goto write_errors;<br>
+ }<br>
</blockquote>
<div><br>
</div>
<div>Why is it necessary to save the variables and
instructions first?<br>
</div>
</div>
</div>
</div>
</blockquote>
<br>
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.<br>
<br>
Thanks for the review;<br>
<br>
// Tapani<br>
<br>
</body>
</html>