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

Tue Sep 5 15:22:24 PDT 2017

Unless it's showing up on a profile, isn't all of this just a solution
looking for a problem?  Davide claims neither the original or updated code
shows up on any profiles, so why don't we just default to readable?

On Tue, Sep 5, 2017 at 3:17 PM Greg Clayton <clayborg at gmail.com> wrote:

> 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 <jmolenda at 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 <clayborg at 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 <dccitaliano at gmail.com>
> wrote:
> >>>
> >>> On Tue, Sep 5, 2017 at 2:03 PM, Jason Molenda <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/d141ca4e/attachment.html>