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?
> On Sep 4, 2017, at 2:22 PM, Davide Italiano via Phabricator
> <[email protected]> wrote:
>
> This revision was automatically updated to reflect the committed changes.
> Closed by commit rL312501: [ABI] Rewrite RegisterIsCalleeSaved. (authored by
> davide).
>
> Changed prior to commit:
> https://reviews.llvm.org/D37420?vs=113675&id=113790#toc
>
> Repository:
> rL LLVM
>
> https://reviews.llvm.org/D37420
>
> Files:
> lldb/trunk/source/Plugins/ABI/SysV-x86_64/ABISysV_x86_64.cpp
>
>
> Index: lldb/trunk/source/Plugins/ABI/SysV-x86_64/ABISysV_x86_64.cpp
> ===================================================================
> --- lldb/trunk/source/Plugins/ABI/SysV-x86_64/ABISysV_x86_64.cpp
> +++ lldb/trunk/source/Plugins/ABI/SysV-x86_64/ABISysV_x86_64.cpp
> @@ -13,6 +13,7 @@
> // C++ Includes
> // Other libraries and framework includes
> #include "llvm/ADT/STLExtras.h"
> +#include "llvm/ADT/StringSwitch.h"
> #include "llvm/ADT/Triple.h"
>
> // Project includes
> @@ -1908,52 +1909,16 @@
> // It's being revised & updated at https://github.com/hjl-tools/x86-psABI/
>
> bool ABISysV_x86_64::RegisterIsCalleeSaved(const RegisterInfo *reg_info) {
> - if (reg_info) {
> - // Preserved registers are :
> - // rbx, rsp, rbp, r12, r13, r14, r15
> - // mxcsr (partially preserved)
> - // x87 control word
> -
> - const char *name = reg_info->name;
> - if (name[0] == 'r') {
> - switch (name[1]) {
> - case '1': // r12, r13, r14, r15
> - if (name[2] >= '2' && name[2] <= '5')
> - return name[3] == '\0';
> - break;
> -
> - default:
> - break;
> - }
> - }
> -
> - // Accept shorter-variant versions, rbx/ebx, rip/ eip, etc.
> - if (name[0] == 'r' || name[0] == 'e') {
> - switch (name[1]) {
> - case 'b': // rbp, rbx
> - if (name[2] == 'p' || name[2] == 'x')
> - return name[3] == '\0';
> - break;
> -
> - case 'i': // rip
> - if (name[2] == 'p')
> - return name[3] == '\0';
> - break;
> -
> - case 's': // rsp
> - if (name[2] == 'p')
> - return name[3] == '\0';
> - break;
> - }
> - }
> - if (name[0] == 's' && name[1] == 'p' && name[2] == '\0') // sp
> - return true;
> - if (name[0] == 'f' && name[1] == 'p' && name[2] == '\0') // fp
> - return true;
> - if (name[0] == 'p' && name[1] == 'c' && name[2] == '\0') // pc
> - return true;
> - }
> - return false;
> + if (!reg_info)
> + return false;
> + assert(reg_info->name != nullptr && "unnamed register?");
> + std::string Name = std::string(reg_info->name);
> + bool IsCalleeSaved =
> + llvm::StringSwitch<bool>(Name)
> + .Cases("r12", "r13", "r14", "r15", "rbp", "ebp", "rbx", "ebx",
> true)
> + .Cases("rip", "eip", "rsp", "esp", "sp", "fp", "pc", true)
> + .Default(false);
> + return IsCalleeSaved;
> }
>
> void ABISysV_x86_64::Initialize() {
>
>
> <D37420.113790.patch>
_______________________________________________
lldb-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
