[Mesa-dev] [PATCH] glcpp: simplify the check for hexadecimal number

[Vlad Golovkin]

2017-06-02 17:59 GMT+03:00 Emil Velikov <emil.l.velikov at gmail.com>:
> On 2 June 2017 at 14:56, Vlad Golovkin <vlad.golovkin.mail at gmail.com> wrote:
>> ---------- Forwarded message ----------
>> From: Vlad Golovkin <vlad.golovkin.mail at gmail.com>
>> Date: 2017-06-02 16:55 GMT+03:00
>> Subject: Re: [Mesa-dev] [PATCH] glcpp: simplify the check for hexadecimal number
>> To: Emil Velikov <emil.l.velikov at gmail.com>
>>
>>
>> 2017-06-02 16:07 GMT+03:00 Emil Velikov <emil.l.velikov at gmail.com>:
>>> Hi Vlad,
>>>
>>> Welcome to Mesa.
>>>
>>> Have you don't any benchmarking on this patch via something like shader-db [1]?
>>> Mentioning the results in the commit message is encouraged.
>>
>> Hi Emil,
>>
>> If I understand correctly shader-db supports only i965 and radeonsi,
>> or am I missing something?
>> I would be glad to be proven wrong, because I only have old r600 gpu.
>>
> In-tree you have helper scripts for i965, nouveau, radeonsi and
> freedreno, but one can run shader-db on any driver.
>

Hi Emil,
Sorry for not responding for so long, I spend a good amount of time
trying to collect enough shaders to get reasonable measurements.
I added some more shaders to my local copy of shader-db, mainly from:
some Firefox WebGL demos, Pcsx2 emulator, Unvanquished, Minetest,
Widelands, Krita and Godot Engine.

During testing I accidentally discovered the bug in this little piece
of code that
I was looking at for quite some time - strcmp doesn't check for "0X"
hex prefix, only for "0x".
I thought that this was because of some flex "magic", which was
providing hex numbers
already lowercased or something like that, but this was not the case.

Right now if you pass shader with the following structure:

#if 0X1 // or any hex number with the 0X prefix
// some code
#endif

the code between #if and #endif gets removed because the check for hex
nubmer fails which
results in strtoll being called with the base 8 and after encountering
the 'X' char the strtoll returns 0.
I think that this bug was undiscovered for so long because hardly
anyone writes hex numbers
with the 0X prefix in the shaders, especially in the preprocessor
directives, for example there are no
such cases in shader-db.

Letting strtoll detect the base makes this limitation go away and also
makes code easier on the eyes (and even marginally faster).

perf results:

master branch
11,978919700 s

old patch
11,963108442 s

strtoll with base 0
11,941502948 s

I can send this new strtoll patch for review.

>>>
>>> On 2 June 2017 at 13:37, Vlad Golovkin <vlad.golovkin.mail at gmail.com> wrote:
>>>> This will make it possible to remove unnecessary strlen and strncmp
>>>> calls.
>>>> ---
>>>>  src/compiler/glsl/glcpp/glcpp-parse.y | 10 ++++++----
>>>>  1 file changed, 6 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/src/compiler/glsl/glcpp/glcpp-parse.y b/src/compiler/glsl/glcpp/glcpp-parse.y
>>>> index fe211a0f0b..8cdd4c9d5b 100644
>>>> --- a/src/compiler/glsl/glcpp/glcpp-parse.y
>>>> +++ b/src/compiler/glsl/glcpp/glcpp-parse.y
>>>> @@ -450,10 +450,12 @@ control_line_error:
>>>>
>>>>  integer_constant:
>>>>         INTEGER_STRING {
>>>> -               if (strlen ($1) >= 3 && strncmp ($1, "0x", 2) == 0) {
>>>> -                       $$ = strtoll ($1 + 2, NULL, 16);
>>>> -               } else if ($1[0] == '0') {
>>>> -                       $$ = strtoll ($1, NULL, 8);
>>>> +               if ($1[0] == '0') {
>>>> +                       if ($1[1] == 'x' && $1[2] != '\0') {
>>>> +                               $$ = strtoll ($1 + 2, NULL, 16);
>>>> +                       } else {
>>>> +                               $$ = strtoll ($1, NULL, 8);
>>>> +                       }
>>>>                 } else {
>>>>                         $$ = strtoll ($1, NULL, 10);
>>>>                 }
>>> Food for thought:
>>>
>>> Any idea why we'd want to handle any of these details ourselves?
>>> strtoll(..., /*base*/ 0) is smart enough, plus it should be better optimised.
>> This is a good approach, it will allow to write just one simple
>> function call instead of all these checks.
>> For some reason i thought that calling strtoll(str, NULL, 0) will make
>> it autodetect the base - from 2 to 36, when in reality it basically
>> tries to match bases 8, 10 and 16.
>>>
>>> Another possibility is to swap the INTEGER_STRING token for
>>> {DEC,OCT,HEX}... ones each one calling strtoll() with correct base.
>> Advantage of this approach, however, is that in this case one can just
>> use custom string to num functions that avoid the overhead of strtoll,
>> but then again it is better to benchmark before drawing any
>> conclusions.
> Indeed. Apart from shader-db, shader intensive games may also help -
> Deus Ex perhaps?
>

> -Emil