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

[Emil Velikov]

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.

>>
>> 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