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
