[Mesa-dev] [PATCH 18/21] glsl: Write a new built-in function module.

[Paul Berry]

On 4 September 2013 15:22, Kenneth Graunke <kenneth at 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 <kenneth at 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 <stereotype441 at gmail.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20130908/d2b80d14/attachment-0001.html>