Ok last message. I bet it's because the patch writes std::string
Name(reg_info->Name);
change this to StringRef and it should be fine. I'd be curious to see how
many instructions that generates.
On Tue, Sep 5, 2017 at 2:12 PM Zachary Turner <ztur...@google.com> wrote:
> 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 <ztur...@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 <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.
>>>
>>>
>>> >
>>> > 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
>>>
>>>
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
