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

Tue Sep 5 15:36:59 PDT 2017

The C-string table and strcmp solution is fine, but I think the
StringSwitch method is strictly better. It's both faster (although I think
we agreed that speed doesn't matter in this case) //and// more readable.

Another alternative would be:

constexpr StringRef Registers[] = {"r12", "r13", "r14", "r15", "rbp",
"rbx", "ebp", "ebx", "rip", "eip", "rsp", "esp", "sp", "fp", "pc"};
bool IsCalleeSaved = llvm::is_contained(Registers, reg_info->name);

On Tue, Sep 5, 2017 at 3:30 PM Jason Molenda <jmolenda at apple.com> wrote:

> I don't think anyone disagrees with that -- the simple c-string table &
> strcmp solution is fine.  It's fun to muse about different approaches like
> this, though.  More generally, I think the way lldb handles registers is
> one of the less well-designed parts of the debugger and it's something I
> often think about in the back of my mind, how it could possibly be done
> better than it is today.  You can see me experimenting with an idea with
> the RegisterNumber class I use in RegisterContextLLDB - this is one tiny
> part of the problem that I was trying to simplify in the unwinder.
>
>
> > On Sep 5, 2017, at 3:22 PM, Zachary Turner <zturner at google.com> wrote:
> >
> > 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/593ce9db/attachment.html>