[Mesa-dev] [PATCH 0/9][RFC] GLSL preprocessor/parser improvements

Sun May 28 13:40:59 UTC 2017

2017-05-22 20:02 GMT+02:00 Thomas Helland <thomashelland90 at gmail.com>:
> 2017-05-22 3:41 GMT+02:00 Timothy Arceri <tarceri at itsqueeze.com>:
>> I suspect you forgot to test with a debug build, there is issues with the
>> string buffer  changes somewhere.
>>
>> ralloc.c:203: reralloc_size: Assertion `ralloc_parent(ptr) == ctx' failed.
>>
>> #0  0x00007ffff70eb91f in raise () from /lib64/libc.so.6
>> #1  0x00007ffff70ed51a in abort () from /lib64/libc.so.6
>> #2  0x00007ffff70e3da7 in __assert_fail_base () from /lib64/libc.so.6
>> #3  0x00007ffff70e3e52 in __assert_fail () from /lib64/libc.so.6
>> #4  0x00007ffff442c73c in reralloc_size (ctx=ctx at entry=0x7fffd40cb910,
>> ptr=<optimized out>, size=<optimized out>) at ralloc.c:203
>> #5  0x00007ffff44b519e in glcpp_preprocess
>> (ralloc_ctx=ralloc_ctx at entry=0x7fffd40c9da0,
>> shader=shader at entry=0x7fffea7cfcc8, info_log=info_log at entry=0x7fffd40ca058,
>>     extensions=extensions at entry=0x7ffff43ff070
>> <add_builtin_defines(_mesa_glsl_parse_state*, void (*)(glcpp_parser*, char
>> const*, int), glcpp_parser*, unsigned int, bool)>,
>> state=state at entry=0x7fffd40c9da0,
>>     gl_ctx=gl_ctx at entry=0x7fffe85c7010) at glsl/glcpp/pp.c:246
>> #6  0x00007ffff4402665 in _mesa_glsl_compile_shader
>> (ctx=ctx at entry=0x7fffe85c7010, shader=shader at entry=0x7fffd40c8cc0,
>> dump_ast=dump_ast at entry=false, dump_hir=dump_hir at entry=false,
>> force_recompile=force_recompile at entry=false)
>>     at glsl/glsl_parser_extras.cpp:2065
>>
>>
>
> Your suspicion is of course correct. Switching back and forth between
> profiling and coding and I forgot to switch debug back on. I'll look into
> this, plus addressing all of the nice feedback. Thanks for taking a look.
>

I've tracked down the issue and sent some updates to the list.
The issue was related to trying to realloc with the wrong
ralloc context in pp.c when resizing the buffer to reduce footprint.
I've added a "crimp_to_fit" function that does exactly that.
This is used instead of handrolling it in pp.c like was initially done.

I've also worked on porting the tests to gtest. It compiles nicely,
I just have to figure out how to run the gtest tests... It shouldn't
be to far out though, so expect them on the list the coming week or so.

Some performance numbers when running shader-db with
GALLIUM_NOOP set on a nouveau gallium driver:
Runtime drops from 48 to 45 seconds.
8% reduction in executed instruction, 6-7% drop in cycles.
So we are seeing that stalled cycles due to cache misses
are a primary limiting factor that's holding us back.
In other words, nothing new under the sun.

>>
>> On 22/05/17 06:49, Thomas Helland wrote:
>>>
>>> This patch series contains some of the work done by Vladislav
>>> in the beginning of March, that seems to have been forgotten.
>>>
>>> For reference, that series, with review comments, can be found here:
>>> https://lists.freedesktop.org/archives/mesa-dev/2017-January/139892.html
>>>
>>> I've adressed some of the review comments, most notably redoing the
>>> string buffer implementation quite a bit, and adding tests.
>>> The major change is that it is now decoupled from the parser
>>> and instead lives in /src/util/ as a utility for easier reuse.
>>> I've also closely followed the structure of our hash table and set
>>> so that it should be quite familiar for everyone. My implementation
>>> is also null terminated, while Vladislav's implementation was not.
>>> My reasoning behind this was that it's less fragile if someone where
>>> to access the contents of the buffer directly. I've also expanded
>>> with some different functions that allow us to do more stuff.
>>> Obviously this could be expanded upon further if need be.
>>>
>>> I've included some of Vladislav's less involved patches that
>>> I think we should get in as soon as possible. I've added Ian's
>>> r-b on the ones that he reviewed (I hope that's OK). The one
>>> patch that is missing from this series is the hand-written
>>> custom parser. While this was the thing that gave the biggest
>>> speed-up, unfortunately it's also a bit harder to review,
>>> so I've left that as a future exercise.
>>>
>>> I've run it through my complete shader-db and there's no changes
>>> or breakages, so that's encouraging. It amounts to about a 3%
>>> reduction in runtime and executed instructions on the shader-db run.
>>> It should be noted that the i965 backend is the primary bottleneck
>>> when running shader-db on my computer, so the numbers don't look all
>>> that impressive due to this. I'll see if I can come up with some
>>> numbers using the gallium noop driver.
>>>
>>> I have not done a very thorough job on testing that the output
>>> of the preprocessor/parser is the same after this series.
>>> However, there's no changes here that I believe should cause
>>> any issues, so I feel quite confident that this series should
>>> not cause any trouble. The original series did have some issues
>>> discovered by running a fuzzer over it, however I expect that
>>> was caused by the introduced hand-written fast-path scanner and
>>> associated macro substitutaion that are not a part of this series.
>>>
>>> CC: Vladislav Egorov <vegorov180 at gmail.com>
>>> CC: Ian Romanick <idr at freedesktop.org>
>>>
>>> Thomas Helland (5):
>>>    util: Add a string buffer implementation
>>>    util: Add tests for the string buffer
>>>    glsl: Change the parser to use the string buffer
>>>    glcpp: Use string_buffer for line continuation removal
>>>    glcpp: Avoid unnecessary call to strlen
>>>
>>> Vladislav Egorov (4):
>>>    glcpp: Avoid unnecessary strcmp()
>>>    glcpp: Skip unnecessary line continuations removal
>>>    ralloc: Use strnlen() inside of strncat()
>>>    glcpp: Use Bloom filter before identifier search
>>>
>>>   configure.ac                                    |   1 +
>>>   src/compiler/glsl/glcpp/glcpp-lex.l             |   3 +-
>>>   src/compiler/glsl/glcpp/glcpp-parse.y           | 123 +++++++++-------
>>>   src/compiler/glsl/glcpp/glcpp.h                 |  18 ++-
>>>   src/compiler/glsl/glcpp/pp.c                    |  73 ++++++----
>>>   src/util/Makefile.am                            |   3 +-
>>>   src/util/Makefile.sources                       |   2 +
>>>   src/util/ralloc.c                               |   7 +-
>>>   src/util/string_buffer.c                        | 180
>>> ++++++++++++++++++++++++
>>>   src/util/string_buffer.h                        |  75 ++++++++++
>>>   src/util/tests/string_buffer/Makefile.am        |  34 +++++
>>>   src/util/tests/string_buffer/append_and_print.c |  82 +++++++++++
>>>   12 files changed, 505 insertions(+), 96 deletions(-)
>>>   create mode 100644 src/util/string_buffer.c
>>>   create mode 100644 src/util/string_buffer.h
>>>   create mode 100644 src/util/tests/string_buffer/Makefile.am
>>>   create mode 100644 src/util/tests/string_buffer/append_and_print.c
>>>
>>