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

[Jason Molenda via lldb-commits]

> 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