[llvm-commits] PATCH: replace if-chain in AsmParser's directive parsing code

[Eli Bendersky]

Ping?

On Tue, Dec 18, 2012 at 2:05 PM, Eli Bendersky <eliben at google.com> wrote:
>>> AsmParser has a couple of if-chains when parsing directives. The
>>> attached patch replaces the if-chains with enum+switch. No change in
>>> functionality.
>>>
>>> Unfortunately I didn't measure any significant speedups for the whole
>>> llvm-mc (only 1-2%), so I'll do more fine-grained measurements once I
>>> get the opportunity.
>>>
>>> Also, I should add that in general AsmParser's parsing code can be
>>> cleaned up even more (for example w.r.t. the extensions registersing
>>> directive parsers, which can be unified with other directive parsing
>>> code). So this patch should just be seen as a first step in a
>>> (hopefully) right direction.
>>
>> Just MHO, but this makes the code more complex and hopefully won't provide a speedup.
>> We generally expect that:
>>
>> StringRef X = …
>>
>> if (X == "foo")
>>   return ...
>> if (x == "bar")
>>   return …
>>
>> to be turned into a switch on X[0].
>>
>
> I may be missing something, but this code:
>
> int main(int argc, char** argv) {
>   StringRef r(argv[1]);
>
>   if (r == "something")
>     return 17;
>   if (r == "foo")
>     return 31;
>   if (r == "bar")
>     return 71;
>   if (r == "baz")
>     return 97;
>
>   return 0;
> }
>
> When compiled with 'clang -O3' (Clang 3.2) does not produce a jump
> table, but a chain of tests and jumps as usual from an if-chain (**).
> I tried several variations (else-ifs...), and the result is similar.
> By the way, this is also what GCC does (and I suspect that many
> developers have their LLVM compiled with GCC).
> Perhaps I am missing something?
>
> Besides, the actual pattern in AsmParser is not a pure "if X return Y"
> chain. Some "then" statements don't return, and some conditions are
> logical ANDs of several tests, and so on. So detecting a clean switch
> pattern could be difficult.
>
>> Just MHO, but this makes the code more complex and hopefully won't provide a speedup.
>
> To address the "more complex" point. Whimsically, it would be hard to
> make that code path in AsmParser more complex ;-) But seriously, a
> longer-term plan would be to combine this lookup table with another
> that already exists in the same function - DirectiveMap, which is used
> for assembler extensions registering themselves on certain directives.
> My thinking was to first get the general if-chain removal approved,
> and then try to fuse the two lookup tables to attain some level of
> consistency in the code. IMHO the end result would be much cleaner and
> more readable than what it has now.
>
> (**) Methodology:
>
> $ ~/llvm/3.2rc2/bin/clang++ -fno-rtti -O3 stringref_switch.cpp
> `~/llvm/3.2rc2/bin/llvm-config --cxxflags --libs`
> `~/llvm/3.2rc2/bin/llvm-config --ldflags` -o stringref_switch
>
> Followed by objdump -d. Here's a typical piece of assembly output:
>
>   4005c0:       53                      push   %rbx
>   4005c1:       48 8b 5e 08             mov    0x8(%rsi),%rbx
>   4005c5:       48 89 df                mov    %rbx,%rdi
>   4005c8:       e8 e3 fe ff ff          callq  4004b0 <strlen at plt>
>   4005cd:       48 89 c1                mov    %rax,%rcx
>   4005d0:       48 83 f9 03             cmp    $0x3,%rcx
>   4005d4:       75 60                   jne    400636 <main+0x76>
>   4005d6:       48 8d 35 89 01 00 00    lea    0x189(%rip),%rsi
> # 400766 <_IO_stdin_used+0xe>
>   4005dd:       48 89 df                mov    %rbx,%rdi
>   4005e0:       48 89 ca                mov    %rcx,%rdx
>   4005e3:       e8 d8 fe ff ff          callq  4004c0 <memcmp at plt>
>   4005e8:       89 c1                   mov    %eax,%ecx
>   4005ea:       b8 1f 00 00 00          mov    $0x1f,%eax
>   4005ef:       85 c9                   test   %ecx,%ecx
>   4005f1:       74 6f                   je     400662 <main+0xa2>
>   4005f3:       48 8d 35 70 01 00 00    lea    0x170(%rip),%rsi
> # 40076a <_IO_stdin_used+0x12>
>   4005fa:       48 89 df                mov    %rbx,%rdi
>   4005fd:       ba 03 00 00 00          mov    $0x3,%edx
>   400602:       e8 b9 fe ff ff          callq  4004c0 <memcmp at plt>
>   400607:       89 c1                   mov    %eax,%ecx
>   400609:       b8 47 00 00 00          mov    $0x47,%eax
>   40060e:       85 c9                   test   %ecx,%ecx
>   400610:       74 50                   je     400662 <main+0xa2>
>   400612:       48 8d 35 55 01 00 00    lea    0x155(%rip),%rsi
> # 40076e <_IO_stdin_used+0x16>
>   400619:       48 89 df                mov    %rbx,%rdi
>   40061c:       ba 03 00 00 00          mov    $0x3,%edx
>   400621:       e8 9a fe ff ff          callq  4004c0 <memcmp at plt>
>   400626:       89 c1                   mov    %eax,%ecx
>   400628:       ba 61 00 00 00          mov    $0x61,%edx
>   40062d:       31 c0                   xor    %eax,%eax
>
> I do notice that the compiler tries to be smart about the length of
> the string, but for strings of the same length it has no choice but
> sequentially "memcmp" them. Note that the vast majority of assembly
> directives fall into very few bins in terms of string length.
>
> Eli