[Mesa-dev] GLSL built-in functions rewrite

[Paul Berry]

On 8 September 2013 10:05, Paul Berry <stereotype441 at gmail.com> wrote:

> On 4 September 2013 15:22, Kenneth Graunke <kenneth at whitecape.org> wrote:
>
>> Hello!
>>
>> This series (at long last!) rewrites the GLSL built-in function handling,
>> for the third time.  It's available on the builtins-v3 branch of
>> ~kwg/mesa.
>>
>> The existing built-in function code had a lot of drawbacks:
>>
>> 1. Writing new built-ins in the S-Expression style IR was painful:
>>
>>    Something as simple as x^2 + 1 turned into:
>>
>>    (expression vec4 + (expression vec4 * (var_ref x) (var_ref x))
>>                       (constant float (1)))
>>
>>    which is extremely wordy, and makes it difficult to follow the
>>    algorithm.  IR builder's add(mul(x, x), imm(1.0f)) is much nicer.
>>
>> 2. Writing new built-ins in GLSL kind of worked some of the time:
>>
>>    To combat the above, we added support for writing built-ins in GLSL.
>>    Unfortunately, they couldn't reliably use other built-in functions,
>>    including essentials like dot(), so this approach was pretty limited.
>>
>> 3. Myriads of cut and pasted prototypes:
>>
>>    Adding support for a new GLSL version meant cut and pasting the
>> previous
>>    version's prototypes, and adjusting a few tiny things.  This resulted
>> in
>>    thousands of duplicated lines, which as we support more and more GLSL
>>    versions means propagating bug fixes is a nightmare.
>>
>> 4. Poor control over built-in availability:
>>
>>    Whether foo() is available depends on the language version, shader
>> stage,
>>    and enabled extensions...in some arbitrary combination.  The old system
>>    worked for simple constraints, but didn't adequately handle things like
>>    "extensions A /and/ B enabled, or extension A and shading language
>> X..."
>>
>> 5. Build system nightmare.
>>
>>    First, it had to build builtin_compiler for the host architecture.
>>  Next,
>>    it would invoke Python scripts to read IR files, invoke
>> builtin_compiler,
>>    and generate a several megabyte C++ source file containing giant
>> strings
>>    of S-Expressions.  Then it had to recompile the compiler with the
>> actual
>>    built-in functions, this time for the target architecture.  When not
>>    cross-compiling, it would skip some of the recompiling.
>>
>>    We had to make all that work with Autotools, SCons, and Android.
>>
>>    We broke cross-compiling so many times.
>>
>>    When you inevitably broke the GLSL compiler, you couldn't even compile
>>    Mesa (the built-ins wouldn't compile), making debugging a challenge.
>>
>> 6. Slow startup time.
>>
>>    When requesting a built-in function, we had to parse the strings into
>>    S-Expressions, then parse those to generate actual IR.  This took a
>> while.
>>
>> 7. Finding a matching built-in was complicated.
>>
>>    Built-ins were spread across multiple gl_shader objects, so the code to
>>    find a matching signature had to loop over all of them.  Worse, one of
>>    the early shaders might define an inexact match, while a later one had
>>    an exact match.  Multiple passes, huzzah!
>>
>> But I digress.
>>
>> This series takes a significantly different approach: it implements the
>> built-in functions directly in C++, using ir_builder.  All signatures are
>> stored in a single shader object.  Availability is expressed via boolean
>> predicates, which can be arbitrary expressions using
>> _mesa_glsl_parse_state
>> fields.  Unavailable functions are simply filtered out when looking for
>> matching built-ins and importing prototypes.
>>
>> It's significantly less code:
>>
>>    166 files changed, 3774 insertions(+), 11918 deletions(-)
>>
>>    (we could possibly scrap more...the S-Expression code and IR reader
>>     still exist for unit testing purposes.)
>>
>> It's also faster:
>>
>>    $ piglit-run.py -t glslparsertest tests/quick.tests r
>>
>> takes about 12% less time.
>>
>> There's no crazy two-stage compilation process: no builtin_compiler,
>> no Python scripts, no cross compiling headaches.
>>
>> I'd appreciate any feedback you might have.  Testing with non-Autotools
>> would be appreciated as well.
>>
>> I found no Piglit or Khronos ES3 conformance regressions on i965/IVB.
>>
>> --Ken
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>>
>
> I sent comments on patches 1, 4, 5, and 18.  Patches 20 and 21 are:
>
> Acked-by: Paul Berry <stereotype441 at gmail.com>
>
> (20 because I'm not an expert on Mesa's build system, 21 because I didn't
> delve into detail to make sure you had deleted exactly the right set of
> files).
>
> The remainder are:
>
> Reviewed-by: Paul Berry <stereotype441 at gmail.com>
>
> My one major reservation about the series is the texturing functions.  I
> doubt anyone is going to review them carefully, and we know that there is
> woefully inadequate piglit testing for texturing.  But I still think it's
> worth moving forward with this series because:
>
> (a) it's such a tremendous improvement on what we have now that I'm
> willing to put up with the risk of introducing some texturing bugs for the
> sake of this much code cleanup.
>
> (b) we are at a good point in the release cycle to make a change like
> this--there is a lot of time for texturing bugs to be discovered and fixed.
>
> (c) if we do discover texturing bugs between now and the next release,
> that will be a good opportunity to improve our piglit testing of texturing
> functions.
>

Reflecting on this further, here's something we could do that would
eliminate my reservation, and give me enough confidence to give patch 18 a
full reviewed-by:

- Add a temporary hack to the current master that causes the build system
to output the IR for every possible built-in (using ir_print_visitor), in
some canonical order.
- Add a temporary hack to your builtins-v3 branch that causes
builtin_builder to output the IR for every possible built-in (using
ir_print_visitor), in the same canonical order.
- Diff the two outputs.

What do you think?  Would that be feasible enough to be worth trying?
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20130908/c72a3d6b/attachment-0001.html>