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

[Zachary Turner via lldb-commits]

I noticed you said it generates new and delete.  I find that strange, we
should look into why that's happening because it's not supposed to be.

On Tue, Sep 5, 2017 at 2:07 PM Zachary Turner <zturner at google.com> wrote:

> 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/c32a6f71/attachment.html>