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

[Zachary Turner via lldb-commits]

StringSwitch doesn't create any std::strings (doing so would allocate
memory), but it does do the memcmp.  Unless it's in a hot path I think
optimizing for readability is the right choice.

On Tue, Sep 5, 2017 at 2:02 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.
>
>
> >
> > With that in mind, I'm under the impression your solution would work.
> > An alternative would be that of looking at clang codegen for
> > StringSwitch and see whether it generates this code.
> > FWIW, I expected that function to be lowered to a switch (in LLVM IR)
> > and in case it has too many cases, being transformed back into a loop.
> > I guess this actually doesn't work as LLVM doesn't really try to
> > modify libcalls lying around too much, and the optimizer can't reason
> > about this case.
> > I guess it will be an interesting thing to look regardless, but the
> > solution you propose seems better, at least in the sense that doesn't
> > rely on the compiler to generate particularly good code to be
> > efficient.
> >
> > Do you want me to apply this patch & run the regression test suite?
> >
> > Thanks,
> >
> > --
> > Davide
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20170905/7042a77d/attachment.html>