We should actually populate the register info with a cached value of this so we
do this once per process?
> On Sep 5, 2017, at 3:12 PM, Jason Molenda <jmole...@apple.com> wrote:
>
> This method gets called every time we try to read a register in the unwinder
> when we're above stack frame 0. The unwinder won't try to fetch volatile
> (non-callee-spilled) registers, and it uses this ABI method to check before
> trying to retrieve the reg.
>
> We could switch the preserved register name table to be ConstString's and pay
> a one-time encoding cost for them, but the 'struct RegisterInfo' stores its
> register name as a c-string so we'd need to encode that into a ConstString
> every time we enter the method.
>
> (or change RegisterInfo to have a ConstString rep of the register name
> instead of a c-string. which wouldn't be a bad idea.)
>
>
>> On Sep 5, 2017, at 3:04 PM, Greg Clayton <clayb...@gmail.com> wrote:
>>
>> 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 <dccitali...@gmail.com> wrote:
>>>
>>> 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
