<div dir="ltr">On 4 September 2013 15:22, Kenneth Graunke <span dir="ltr"><<a href="mailto:kenneth@whitecape.org" target="_blank">kenneth@whitecape.org</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">This creates a new replacement for the existing built-in function code.<br>
The new module lives in builtin_functions.cpp (not builtin_function.cpp)<br>
and exists in parallel with the existing system. It isn't used yet.<br>
<br>
The new built-in function code takes a significantly different approach:<br>
<br>
Instead of implementing built-ins via printed IR, build time scripts,<br>
and run time parsing, we now implement them directly in C++, using<br>
ir_builder. This translates to faster load times, and a much less<br>
complex build system.<br>
<br>
It also takes a different approach to built-in availability: each<br>
signature now stores a boolean predicate, which makes it easy to<br>
construct arbitrary expressions based on _mesa_glsl_parse_state's<br>
fields. This is much more flexible than the old system, and also<br>
easier to use.<br>
<br>
Built-ins are also now stored in a single gl_shader object, rather<br>
than being spread out across a number of shaders that need to be linked.<br>
When searching for a matching prototype, we simply consult the<br>
availability predicate. This also simplifies the code.<br>
<br>
Signed-off-by: Kenneth Graunke <<a href="mailto:kenneth@whitecape.org">kenneth@whitecape.org</a>><br>
---<br>
src/glsl/Makefile.sources | 1 +<br>
src/glsl/builtin_functions.cpp | 3466 ++++++++++++++++++++++++++++++++++++++++<br>
src/glsl/ir.h | 10 +<br>
3 files changed, 3477 insertions(+)<br>
create mode 100644 src/glsl/builtin_functions.cpp<br>
<br></blockquote><div><br></div><div><snip><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+static bool<br>
+legacy_vs_only(const _mesa_glsl_parse_state *state)<br>
+{<br>
+ return state->target == vertex_shader &&<br>
+ state->language_version <= 130 &&<br>
+ !state->es_shader;<br>
+}<br></blockquote><div><br></div><div>Elsewhere in src/glsl, we use the term "compatibility" instead of "legacy". Maybe rename for consistency?<br><br></div><div><snip><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+class builtin_builder {<br>
+public:<br>
+ builtin_builder();<br>
+ ~builtin_builder();<br>
+<br>
+ void initialize();<br>
+ void release();<br>
+ ir_function_signature *find(_mesa_glsl_parse_state *state,<br>
+ const char *name, exec_list *actual_parameters);<br>
+<br>
+private:<br>
+ void *mem_ctx;<br>
+ gl_shader *shader;<br></blockquote><div><br></div><div>It would be nice to have a comment here explaining that "shader" is the shader that we're building to hold all the built-ins, not a user-defined shader that's getting compiled.<br>
<br></div><div><snip><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ir_function_signature *<br>
+builtin_builder::find(_mesa_glsl_parse_state *state,<br>
+ const char *name, exec_list *actual_parameters)<br>
+{<br>
+ /* The shader needs to link against the built-ins. */<br>
+ state->builtins_to_link[0] = shader;<br></blockquote><div><br></div><div>This is confusing because the word "shader" in the comment refers to something different from the variable "shader". Maybe something like this instead?<br>
<br></div><div>/* The shader being compiled needs to link against the built-ins contained in "shader" */<br><br></div><div><snip><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+extern "C" struct gl_shader *<br>
+_mesa_new_shader(struct gl_context *ctx, GLuint name, GLenum type);<br></blockquote><div><br></div><div>Why not #include "main/shaderobj.h" to get access to this function?<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+<br>
+void<br>
+builtin_builder::create_shader()<br>
+{<br>
+ shader = _mesa_new_shader(NULL, 0, GL_VERTEX_SHADER);<br></blockquote><div><br></div><div>It would be nice to have a comment explaining why it's ok to pass GL_VERTEX_SHADER here, even though we're creating a shader object to hold all built-ins for all shader stages.<br>
<br></div><div><snip><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">+#define FIU(NAME) \<br>
+ add_function(#NAME, \<br>
+ _##NAME(glsl_type::float_type), \<br>
+ _##NAME(glsl_type::vec2_type), \<br>
+ _##NAME(glsl_type::vec3_type), \<br>
+ _##NAME(glsl_type::vec4_type), \<br>
+ \<br>
+ _##NAME(glsl_type::int_type), \<br>
+ _##NAME(glsl_type::ivec2_type), \<br>
+ _##NAME(glsl_type::ivec3_type), \<br>
+ _##NAME(glsl_type::ivec4_type), \<br>
+ \<br>
+ /* XXX: need to make uints only available in 1.30+ */ \<br></blockquote><div><br></div><div>Is this still an open issue? It seems like we ought to come up with a plan for addressing it before we land the series.<br>
</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ _##NAME(glsl_type::uint_type), \<br>
+ _##NAME(glsl_type::uvec2_type), \<br>
+ _##NAME(glsl_type::uvec3_type), \<br>
+ _##NAME(glsl_type::uvec4_type), \<br>
+ NULL);<br>
+<br></blockquote><div><br></div><div><snip><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">+<br>
+ add_function("smoothstep",<br>
+ _smoothstep(glsl_type::float_type, glsl_type::float_type),<br>
+ _smoothstep(glsl_type::float_type, glsl_type::vec2_type),<br>
+ _smoothstep(glsl_type::float_type, glsl_type::vec3_type),<br>
+ _smoothstep(glsl_type::float_type, glsl_type::vec4_type),<br>
+<br>
+ _smoothstep(glsl_type::vec2_type, glsl_type::vec2_type),<br>
+ _smoothstep(glsl_type::vec3_type, glsl_type::vec3_type),<br>
+ _smoothstep(glsl_type::vec4_type, glsl_type::vec4_type),<br>
+ NULL);<br>
+<br>
</blockquote><div><br></div><div>There's some stray whitespace around "smoothstep".<br></div><div> </div><div><snip><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+void<br>
+builtin_builder::add_function(const char *name, ...)<br>
+{<br>
+ va_list ap;<br>
+<br>
+ ir_function *f = new(mem_ctx) ir_function(name);<br>
+<br>
+ va_start(ap, name);<br>
+ while (true) {<br>
+ ir_function_signature *sig = va_arg(ap, ir_function_signature *);<br>
+ if (sig == NULL)<br>
+ break;<br>
+<br>
+ sig->is_defined = true;<br>
+<br>
+#if 0<br>
+ exec_list stuff;<br>
+ stuff.push_tail(sig);<br>
+ validate_ir_tree(&stuff);<br>
+#endif<br></blockquote><div><br></div><div>I prefer to use "if (false)" for disabled debugging code like this--it reduces bit rot since it ensures that the disabled code remains compileable. <br></div><div> </div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+<br>
+ f->add_signature(sig);<br>
+ }<br></blockquote><div><br></div><div>Can we add "assert(sig->is_defined);" here? If add_function() ever gets called with zero non-null varargs, that's definitely a bug.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+<br>
+ shader->symbols->add_function(f);<br>
+}<br>
+<br></blockquote><div><br></div><div>This is really nice work, Ken. I can't wait to see it land! I don't think I can give it a reviewed-by in good conscience, since there are way too many fidgety little details I didn't have a chance to re-check, but with the issues above addressed, consider it:<br>
<br>Enthusiastically acked-by: Paul Berry <<a href="mailto:stereotype441@gmail.com">stereotype441@gmail.com</a>> <br></div></div></div></div>