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

[Davide Italiano via lldb-commits]

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].

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