On Tue, Sep 5, 2017 at 2:03 PM, Jason Molenda <jmole...@apple.com> wrote:
>
>
>> 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.
>
Yes, I agree, I'll update my checkout and push that change. I'll also
try to see if we can standardize a style across all the ABIs, and what
you propose seems the right tradeoff between perf and readability.
It's unfortunate that StringSwitch generates less than ideal code
here, I guess the concept of zero-cost abstraction needs to be refined
for this very abstraction. I'll open an LLVM bug and try to take a
look.
Thanks for taking the time to look at this further.
--
Davide
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
