> On Sep 5, 2017, at 1:34 PM, Davide Italiano <dccitali...@gmail.com> wrote:
>
> On Tue, Sep 5, 2017 at 1:23 PM, Jason Molenda <jmole...@apple.com> wrote:
>> Hi Davide, sorry I was offline for this discussion.
>>
>> I was a little curious about llvm::StringSwitch, I hadn't seen it before.
>> Is it creating std::string's for all of these strings, then memcmp'ing the
>> contents? Greg originally wrote these RegisterIsCalleeSaved() methods in
>> the ABI's hand optimizing the character comparisons for efficiency,
>> sacrificing readability in the process big-time but we added the comments to
>> make it easier to follow.
>>
>> This version is much easier to read but loses the efficiency. Looking at
>> the assembly generated by clang -Os, we're getting the same of the input
>> string and then running memcmp() against all of the c-strings.
>>
>> If we're going to ignore the efficiency that Greg was shooting for here, why
>> not write it with an array of c-strings and strcmp, like
>>
>> const char *preserved_registers[] = { "r12", "r13", "r14", "r15", "rbp",
>> "ebp", "rbx", "ebx",
>> "rip", "eip", "rsp", "esp", "sp", "fp", "pc", NULL };
>>
>> for (int i = 0; preserved_registers[i] != NULL; i++)
>> if (strcmp (reg, preserved_registers[i]) == 0)
>> return true
>> return false;
>>
>> ?
>>
>>
>> The original version, as hard to read as it was, compiles down to 60
>> instructions with no memory allocations or function calls with clang -Os.
>> Using llvm::StringSwitch is 184, with new, delete, memcpy, memcmp function
>> calls. The strcmp() one weighs in around 30-35 instructions with calls to
>> strcmp.
>>
>> I don't think this function is especially hot, I don't know if Greg's
>> original focus on performance here was really the best choice. But if we're
>> going to give up some performance, we could go the more generic strmp()
>> route just as easily, couldn't we?
>>
>
> Hi Jason,
> I hoped to receive comments, so, thank you. I profiled lldb a bit
> recently and I never saw this function showing up in the profile.
> That said, I agree we shouldn't completely give up performances for
> readability in this case. [In particular, I'm more worried about the
> increase in code size].
When I first looked at this function compiled -O0 it was 880 instructions long
and I laughed out loud. :)
I don't feel strongly about this, your change is fine, I was mostly curious if
I was missing something.
I wouldn't want to make extra work for equivalent readability/performance (IMO)
unless you want to. I think many of the other ABI plugins have similar code in
them; if I were changing any others, I would use the simpler loop & strcmp()
method I think.
>
> With that in mind, I'm under the impression your solution would work.
> An alternative would be that of looking at clang codegen for
> StringSwitch and see whether it generates this code.
> FWIW, I expected that function to be lowered to a switch (in LLVM IR)
> and in case it has too many cases, being transformed back into a loop.
> I guess this actually doesn't work as LLVM doesn't really try to
> modify libcalls lying around too much, and the optimizer can't reason
> about this case.
> I guess it will be an interesting thing to look regardless, but the
> solution you propose seems better, at least in the sense that doesn't
> rely on the compiler to generate particularly good code to be
> efficient.
>
> Do you want me to apply this patch & run the regression test suite?
>
> Thanks,
>
> --
> Davide
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
