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

[Jason Molenda via lldb-commits]

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 <reviews at reviews.llvm.org> 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>