[Lldb-commits] [PATCH] D37420: [ABI] Rewrite RegisterIsCalleeSaved

Greg Clayton via lldb-commits lldb-commits at lists.llvm.org
Tue Sep 5 15:04:40 PDT 2017


You could also use a collection of lldb_private::ConstString objects. Comparing those are pointer compares since lldb_private::ConstString unique the strings into a string pool. lldb_private::UniqueCStringMap has a very efficient way to do this if you need an example.

Not sure how many times we call this function. If it is once per target run, then who cares. If it is every time we stop, then we should look a little closer.

Greg

> On Sep 5, 2017, at 2:06 PM, Davide Italiano <dccitaliano at gmail.com> wrote:
> 
> On Tue, Sep 5, 2017 at 2:03 PM, Jason Molenda <jmolenda at apple.com <mailto:jmolenda at apple.com>> wrote:
>> 
>> 
>>> On Sep 5, 2017, at 1:34 PM, Davide Italiano <dccitaliano at gmail.com> wrote:
>>> 
>>> On Tue, Sep 5, 2017 at 1:23 PM, Jason Molenda <jmolenda at 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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20170905/632d07f0/attachment.html>


More information about the lldb-commits mailing list