On 4 September 2013 15:22, Kenneth Graunke <kenn...@whitecape.org> wrote:
> This creates a new replacement for the existing built-in function code.
> The new module lives in builtin_functions.cpp (not builtin_function.cpp)
> and exists in parallel with the existing system. It isn't used yet.
>
> The new built-in function code takes a significantly different approach:
>
> Instead of implementing built-ins via printed IR, build time scripts,
> and run time parsing, we now implement them directly in C++, using
> ir_builder. This translates to faster load times, and a much less
> complex build system.
>
> It also takes a different approach to built-in availability: each
> signature now stores a boolean predicate, which makes it easy to
> construct arbitrary expressions based on _mesa_glsl_parse_state's
> fields. This is much more flexible than the old system, and also
> easier to use.
>
> Built-ins are also now stored in a single gl_shader object, rather
> than being spread out across a number of shaders that need to be linked.
> When searching for a matching prototype, we simply consult the
> availability predicate. This also simplifies the code.
>
> Signed-off-by: Kenneth Graunke <kenn...@whitecape.org>
> ---
> src/glsl/Makefile.sources | 1 +
> src/glsl/builtin_functions.cpp | 3466
> ++++++++++++++++++++++++++++++++++++++++
> src/glsl/ir.h | 10 +
> 3 files changed, 3477 insertions(+)
> create mode 100644 src/glsl/builtin_functions.cpp
>
>
<snip>
> +static bool
> +legacy_vs_only(const _mesa_glsl_parse_state *state)
> +{
> + return state->target == vertex_shader &&
> + state->language_version <= 130 &&
> + !state->es_shader;
> +}
>
Elsewhere in src/glsl, we use the term "compatibility" instead of
"legacy". Maybe rename for consistency?
<snip>
> +class builtin_builder {
> +public:
> + builtin_builder();
> + ~builtin_builder();
> +
> + void initialize();
> + void release();
> + ir_function_signature *find(_mesa_glsl_parse_state *state,
> + const char *name, exec_list
> *actual_parameters);
> +
> +private:
> + void *mem_ctx;
> + gl_shader *shader;
>
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.
<snip>
> +ir_function_signature *
> +builtin_builder::find(_mesa_glsl_parse_state *state,
> + const char *name, exec_list *actual_parameters)
> +{
> + /* The shader needs to link against the built-ins. */
> + state->builtins_to_link[0] = shader;
>
This is confusing because the word "shader" in the comment refers to
something different from the variable "shader". Maybe something like this
instead?
/* The shader being compiled needs to link against the built-ins contained
in "shader" */
<snip>
> +extern "C" struct gl_shader *
> +_mesa_new_shader(struct gl_context *ctx, GLuint name, GLenum type);
>
Why not #include "main/shaderobj.h" to get access to this function?
> +
> +void
> +builtin_builder::create_shader()
> +{
> + shader = _mesa_new_shader(NULL, 0, GL_VERTEX_SHADER);
>
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.
<snip>
> +#define FIU(NAME) \
> + add_function(#NAME, \
> + _##NAME(glsl_type::float_type), \
> + _##NAME(glsl_type::vec2_type), \
> + _##NAME(glsl_type::vec3_type), \
> + _##NAME(glsl_type::vec4_type), \
> + \
> + _##NAME(glsl_type::int_type), \
> + _##NAME(glsl_type::ivec2_type), \
> + _##NAME(glsl_type::ivec3_type), \
> + _##NAME(glsl_type::ivec4_type), \
> + \
> + /* XXX: need to make uints only available in 1.30+ */ \
>
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.
> + _##NAME(glsl_type::uint_type), \
> + _##NAME(glsl_type::uvec2_type), \
> + _##NAME(glsl_type::uvec3_type), \
> + _##NAME(glsl_type::uvec4_type), \
> + NULL);
> +
>
<snip>
> +
> + add_function("smoothstep",
> + _smoothstep(glsl_type::float_type, glsl_type::float_type),
> + _smoothstep(glsl_type::float_type, glsl_type::vec2_type),
> + _smoothstep(glsl_type::float_type, glsl_type::vec3_type),
> + _smoothstep(glsl_type::float_type, glsl_type::vec4_type),
> +
> + _smoothstep(glsl_type::vec2_type, glsl_type::vec2_type),
> + _smoothstep(glsl_type::vec3_type, glsl_type::vec3_type),
> + _smoothstep(glsl_type::vec4_type, glsl_type::vec4_type),
> + NULL);
> +
>
There's some stray whitespace around "smoothstep".
<snip>
> +void
> +builtin_builder::add_function(const char *name, ...)
> +{
> + va_list ap;
> +
> + ir_function *f = new(mem_ctx) ir_function(name);
> +
> + va_start(ap, name);
> + while (true) {
> + ir_function_signature *sig = va_arg(ap, ir_function_signature *);
> + if (sig == NULL)
> + break;
> +
> + sig->is_defined = true;
> +
> +#if 0
> + exec_list stuff;
> + stuff.push_tail(sig);
> + validate_ir_tree(&stuff);
> +#endif
>
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.
> +
> + f->add_signature(sig);
> + }
>
Can we add "assert(sig->is_defined);" here? If add_function() ever gets
called with zero non-null varargs, that's definitely a bug.
> +
> + shader->symbols->add_function(f);
> +}
> +
>
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:
Enthusiastically acked-by: Paul Berry <stereotype...@gmail.com>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
