[Mesa-dev] [PATCH 18/21] glsl: Write a new built-in function module.
Kenneth Graunke
kenneth at whitecape.org
Sun Sep 8 20:33:12 PDT 2013
On 09/08/2013 09:57 AM, Paul Berry wrote:
> On 4 September 2013 15:22, Kenneth Graunke <kenneth at whitecape.org
> <mailto: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
> <mailto: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>
Changed to "compatibility_vs_only".
>
> +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.
Added:
/**
* A shader to hold all the built-in signatures; created by this module.
*
* This includes signatures for every built-in, regardless of version or
* enabled extensions. The availability predicate associated with each
* signature allows matching_signature() to filter out the irrelevant ones.
*/
>
> <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" */
Replaced with:
/* The shader currently being compiled requested a built-in function;
* it needs to link against builtin_builder::shader in order to get them.
*
* Even if we don't find a matching signature, we still need to do this so
* that the "no matching signature" error will list potential candidates
* from the available built-ins.
*/
>
> <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?
That would be sensible, wouldn't it. I think this was a vestige of the
old two-phase build insanity.
> +
> +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.
Yeah, we've done this trick since 2010 so I didn't think to add a comment.
It wouldn't hurt though. Added:
/* The target doesn't actually matter. There's no target for generic
* GLSL utility code that could be linked against any stage, so just
* arbitrarily pick GL_VERTEX_SHADER.
*/
> <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.
Fixed by passing in always_available or v130.
>
> + _##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>
Fixed.
>
> +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.
Yeah, I kind of waffled on that...enabling this block will always break,
since ir_validate gets confused by ftransform(). I think it's harmless;
we pass validation aside from that.
Anyway, I've changed it to "if (false)." That is better.
>
> +
> + 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.
I'm unclear how that would help. I'm pretty sure forgetting the NULL
sentinel will have already resulted in a crash before we even get here.
And, it seems an odd assertion given that we immediately set it above.
I'm not terribly excited about the variadic functions for this reason.
If I could have, I would have used anonymous arrays to have type-safety:
http://www.run.montefiore.ulg.ac.be/~martin/resources/kung-f00.html
But that technique doesn't work with MSVC. Piece of garbage compiler.
> +
> + 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
> <mailto:stereotype441 at gmail.com>>
Thanks for the review!
More information about the mesa-dev
mailing list