On 8 September 2013 10:05, Paul Berry <stereotype...@gmail.com> wrote:
> On 4 September 2013 15:22, Kenneth Graunke <kenn...@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@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 <stereotype...@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 <stereotype...@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?
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
