[Lldb-commits] [PATCH] D37295: [lldb] Adjust UpdateExternalModuleListIfNeeded method for the case of *.dwo

[Alexander Shaposhnikov via Phabricator via lldb-commits]

alexshap added a comment.

ping


Repository:
  rL LLVM

https://reviews.llvm.org/D37295



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D37295: [lldb] Adjust UpdateExternalModuleListIfNeeded method for the case of *.dwo

[Greg Clayton via Phabricator via lldb-commits]

clayborg accepted this revision.
clayborg added a comment.
This revision is now accepted and ready to land.

Just add a more descriptive comment that states this exact issue more clearly 
as I didn't understand the issue from the comment that is there now (the 
comment that starts on line 1644).


Repository:
  rL LLVM

https://reviews.llvm.org/D37295



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] r312562 - Fix DW_FORM_strp parsing

[Jan Kratochvil via lldb-commits]

Author: jankratochvil
Date: Tue Sep  5 12:01:01 2017
New Revision: 312562

URL: http://llvm.org/viewvc/llvm-project?rev=312562&view=rev
Log:
Fix DW_FORM_strp parsing

Differential revision: https://reviews.llvm.org/D37441

Modified:
lldb/trunk/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp

Modified: lldb/trunk/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp?rev=312562&r1=312561&r2=312562&view=diff
==
--- lldb/trunk/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp 
(original)
+++ lldb/trunk/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp Tue Sep  
5 12:01:01 2017
@@ -161,9 +161,9 @@ bool DWARFDebugInfoEntry::FastExtract(
   case DW_FORM_strp:
   case DW_FORM_sec_offset:
 if (cu->IsDWARF64())
-  debug_info_data.GetU64(offset_ptr);
+  debug_info_data.GetU64(&offset);
 else
-  debug_info_data.GetU32(offset_ptr);
+  debug_info_data.GetU32(&offset);
 break;
 
   default:
@@ -325,9 +325,9 @@ bool DWARFDebugInfoEntry::Extract(Symbol
   case DW_FORM_strp:
   case DW_FORM_sec_offset:
 if (cu->IsDWARF64())
-  debug_info_data.GetU64(offset_ptr);
+  debug_info_data.GetU64(&offset);
 else
-  debug_info_data.GetU32(offset_ptr);
+  debug_info_data.GetU32(&offset);
 break;
 
   default:


___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [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 
>  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(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() {
> 
> 
> 

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Davide Italiano via lldb-commits]

On Tue, Sep 5, 2017 at 1:23 PM, Jason Molenda  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
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Jason Molenda via lldb-commits]

> On Sep 5, 2017, at 1:34 PM, Davide Italiano  wrote:
> 
> On Tue, Sep 5, 2017 at 1:23 PM, Jason Molenda  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

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

[Davide Italiano via lldb-commits]

On Tue, Sep 5, 2017 at 2:03 PM, Jason Molenda  wrote:
>
>
>> On Sep 5, 2017, at 1:34 PM, Davide Italiano  wrote:
>>
>> On Tue, Sep 5, 2017 at 1:23 PM, Jason Molenda  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.
>

Yes, I agree, I'll update my checkout and push that change. I'll also
try to see if we can standardize a style across all the ABIs, and what
you propose seems the right tradeoff between perf and readability.
It's unfortunate that StringSwitch generates less than ideal code
here, I guess the concept of zero-cost abstraction needs to be refined
for this very abstraction. I'll open an LLVM bug and try to take a
look.

Thanks for taking the time to look at this further.

--
Davide
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Zachary Turner via lldb-commits]

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  wrote:

>
>
> > On Sep 5, 2017, at 1:34 PM, Davide Italiano 
> wrote:
> >
> > On Tue, Sep 5, 2017 at 1:23 PM, Jason Molenda 
> 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

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

[Zachary Turner via lldb-commits]

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  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  wrote:
>
>>
>>
>> > On Sep 5, 2017, at 1:34 PM, Davide Italiano 
>> wrote:
>> >
>> > On Tue, Sep 5, 2017 at 1:23 PM, Jason Molenda 
>> 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

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

[Zachary Turner via lldb-commits]

Ok last message.  I bet it's because the patch writes std::string
Name(reg_info->Name);

change this to StringRef and it should be fine.  I'd be curious to see how
many instructions that generates.

On Tue, Sep 5, 2017 at 2:12 PM Zachary Turner  wrote:

> 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  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  wrote:
>>
>>>
>>>
>>> > On Sep 5, 2017, at 1:34 PM, Davide Italiano 
>>> wrote:
>>> >
>>> > On Tue, Sep 5, 2017 at 1:23 PM, Jason Molenda 
>>> 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

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

[Greg Clayton via lldb-commits]

You could also use a collection of lldb_private::ConstString objects. Comparing 
those are pointer compares since lldb_private::ConstString unique the strings 
into a string pool. lldb_private::UniqueCStringMap has a very efficient way to 
do this if you need an example.

Not sure how many times we call this function. If it is once per target run, 
then who cares. If it is every time we stop, then we should look a little 
closer.

Greg

> On Sep 5, 2017, at 2:06 PM, Davide Italiano  wrote:
> 
> On Tue, Sep 5, 2017 at 2:03 PM, Jason Molenda  > wrote:
>> 
>> 
>>> On Sep 5, 2017, at 1:34 PM, Davide Italiano  wrote:
>>> 
>>> On Tue, Sep 5, 2017 at 1:23 PM, Jason Molenda  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.
>> 
> 
> Yes, I agree, I'll update my checkout and push that change. I'll also
> try to see if we can standardize a style across all the ABIs, and what
> you propose seems the right tradeoff between perf and readability.
> It's unfortunate that StringSwitch generates less than ideal code
> here, I guess the concept of zero-cost abstraction needs to be refined
> for this very abstraction. I'll open an LLVM bug and try to take a
> look.
> 
> Thanks for taking the time to look at this further.
> 
> --
> Davide

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Jason Molenda via lldb-commits]

This method gets called every time we try to read a register in the unwinder 
when we're above stack frame 0.  The unwinder won't try to fetch volatile 
(non-callee-spilled) registers, and it uses this ABI method to check before 
trying to retrieve the reg.

We could switch the preserved register name table to be ConstString's and pay a 
one-time encoding cost for them, but the 'struct RegisterInfo' stores its 
register name as a c-string so we'd need to encode that into a ConstString 
every time we enter the method.  

(or change RegisterInfo to have a ConstString rep of the register name instead 
of a c-string.  which wouldn't be a bad idea.)


> On Sep 5, 2017, at 3:04 PM, Greg Clayton  wrote:
> 
> You could also use a collection of lldb_private::ConstString objects. 
> Comparing those are pointer compares since lldb_private::ConstString unique 
> the strings into a string pool. lldb_private::UniqueCStringMap has a very 
> efficient way to do this if you need an example.
> 
> Not sure how many times we call this function. If it is once per target run, 
> then who cares. If it is every time we stop, then we should look a little 
> closer.
> 
> Greg
> 
>> On Sep 5, 2017, at 2:06 PM, Davide Italiano  wrote:
>> 
>> On Tue, Sep 5, 2017 at 2:03 PM, Jason Molenda  wrote:
>>> 
>>> 
 On Sep 5, 2017, at 1:34 PM, Davide Italiano  wrote:
 
 On Tue, Sep 5, 2017 at 1:23 PM, Jason Molenda  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.
>>> 
>> 
>> Yes, I agree, I'll update my checkout and push that change. I'll also
>> try to see if we can standardize a style across all the ABIs, and what
>> you propose seems the right tradeoff between perf and readability.
>> It's unfortunate that StringSwitch generates less than ideal code
>> here, I guess the concept of zero-cost abstraction needs to be refined
>> for this very abstraction. I'll open an LLVM bug and try to take a
>> look.
>> 
>> Thanks for taking the time to look at this further.
>> 
>> --
>> Davide
> 

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Greg Clayton via lldb-commits]

We should actually populate the register info with a cached value of this so we 
do this once per process?

> On Sep 5, 2017, at 3:12 PM, Jason Molenda  wrote:
> 
> This method gets called every time we try to read a register in the unwinder 
> when we're above stack frame 0.  The unwinder won't try to fetch volatile 
> (non-callee-spilled) registers, and it uses this ABI method to check before 
> trying to retrieve the reg.
> 
> We could switch the preserved register name table to be ConstString's and pay 
> a one-time encoding cost for them, but the 'struct RegisterInfo' stores its 
> register name as a c-string so we'd need to encode that into a ConstString 
> every time we enter the method.  
> 
> (or change RegisterInfo to have a ConstString rep of the register name 
> instead of a c-string.  which wouldn't be a bad idea.)
> 
> 
>> On Sep 5, 2017, at 3:04 PM, Greg Clayton  wrote:
>> 
>> You could also use a collection of lldb_private::ConstString objects. 
>> Comparing those are pointer compares since lldb_private::ConstString unique 
>> the strings into a string pool. lldb_private::UniqueCStringMap has a very 
>> efficient way to do this if you need an example.
>> 
>> Not sure how many times we call this function. If it is once per target run, 
>> then who cares. If it is every time we stop, then we should look a little 
>> closer.
>> 
>> Greg
>> 
>>> On Sep 5, 2017, at 2:06 PM, Davide Italiano  wrote:
>>> 
>>> On Tue, Sep 5, 2017 at 2:03 PM, Jason Molenda  wrote:
 
 
> On Sep 5, 2017, at 1:34 PM, Davide Italiano  wrote:
> 
> On Tue, Sep 5, 2017 at 1:23 PM, Jason Molenda  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.
 
>>> 
>>> Yes, I agree, I'll update my checkout and push that change. I'll also
>>> try to see if we can standardize a style across all the ABIs, and what
>>> you propose seems the right tradeoff between perf and readability.
>>> It's unfortunate that StringSwitch generates less than ideal code
>>> here, I guess the concept of zero-cost abstraction needs to be refined
>>> for this very abstraction. I'll open an LLVM bug and try to take a
>>> look.
>>> 
>>> Thanks for taking the time to look at this further.
>>> 
>>> --
>>> Davide
>> 
> 

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

[Zachary Turner via lldb-commits]

Unless it's showing up on a profile, isn't all of this just a solution
looking for a problem?  Davide claims neither the original or updated code
shows up on any profiles, so why don't we just default to readable?

On Tue, Sep 5, 2017 at 3:17 PM Greg Clayton  wrote:

> We should actually populate the register info with a cached value of this
> so we do this once per process?
>
> > On Sep 5, 2017, at 3:12 PM, Jason Molenda  wrote:
> >
> > This method gets called every time we try to read a register in the
> unwinder when we're above stack frame 0.  The unwinder won't try to fetch
> volatile (non-callee-spilled) registers, and it uses this ABI method to
> check before trying to retrieve the reg.
> >
> > We could switch the preserved register name table to be ConstString's
> and pay a one-time encoding cost for them, but the 'struct RegisterInfo'
> stores its register name as a c-string so we'd need to encode that into a
> ConstString every time we enter the method.
> >
> > (or change RegisterInfo to have a ConstString rep of the register name
> instead of a c-string.  which wouldn't be a bad idea.)
> >
> >
> >> On Sep 5, 2017, at 3:04 PM, Greg Clayton  wrote:
> >>
> >> You could also use a collection of lldb_private::ConstString objects.
> Comparing those are pointer compares since lldb_private::ConstString unique
> the strings into a string pool. lldb_private::UniqueCStringMap has a very
> efficient way to do this if you need an example.
> >>
> >> Not sure how many times we call this function. If it is once per target
> run, then who cares. If it is every time we stop, then we should look a
> little closer.
> >>
> >> Greg
> >>
> >>> On Sep 5, 2017, at 2:06 PM, Davide Italiano 
> wrote:
> >>>
> >>> On Tue, Sep 5, 2017 at 2:03 PM, Jason Molenda 
> wrote:
> 
> 
> > On Sep 5, 2017, at 1:34 PM, Davide Italiano 
> wrote:
> >
> > On Tue, Sep 5, 2017 at 1:23 PM, Jason Molenda 
> 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.
> 
> >>>
> >>> Yes, I agree, I'll update my checkout and push that change. I'll also
> >>> try to see if we can standardize a style across all the ABIs, and what
> >>> you propose seems the right tradeoff between perf and readability.
> >>> It's unfortunate that StringSwitch generates less than ideal co

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

[Jason Molenda via lldb-commits]

I don't think anyone disagrees with that -- the simple c-string table & strcmp 
solution is fine.  It's fun to muse about different approaches like this, 
though.  More generally, I think the way lldb handles registers is one of the 
less well-designed parts of the debugger and it's something I often think about 
in the back of my mind, how it could possibly be done better than it is today.  
You can see me experimenting with an idea with the RegisterNumber class I use 
in RegisterContextLLDB - this is one tiny part of the problem that I was trying 
to simplify in the unwinder.


> On Sep 5, 2017, at 3:22 PM, Zachary Turner  wrote:
> 
> Unless it's showing up on a profile, isn't all of this just a solution 
> looking for a problem?  Davide claims neither the original or updated code 
> shows up on any profiles, so why don't we just default to readable?
> 
> On Tue, Sep 5, 2017 at 3:17 PM Greg Clayton  wrote:
> We should actually populate the register info with a cached value of this so 
> we do this once per process?
> 
> > On Sep 5, 2017, at 3:12 PM, Jason Molenda  wrote:
> >
> > This method gets called every time we try to read a register in the 
> > unwinder when we're above stack frame 0.  The unwinder won't try to fetch 
> > volatile (non-callee-spilled) registers, and it uses this ABI method to 
> > check before trying to retrieve the reg.
> >
> > We could switch the preserved register name table to be ConstString's and 
> > pay a one-time encoding cost for them, but the 'struct RegisterInfo' stores 
> > its register name as a c-string so we'd need to encode that into a 
> > ConstString every time we enter the method.
> >
> > (or change RegisterInfo to have a ConstString rep of the register name 
> > instead of a c-string.  which wouldn't be a bad idea.)
> >
> >
> >> On Sep 5, 2017, at 3:04 PM, Greg Clayton  wrote:
> >>
> >> You could also use a collection of lldb_private::ConstString objects. 
> >> Comparing those are pointer compares since lldb_private::ConstString 
> >> unique the strings into a string pool. lldb_private::UniqueCStringMap has 
> >> a very efficient way to do this if you need an example.
> >>
> >> Not sure how many times we call this function. If it is once per target 
> >> run, then who cares. If it is every time we stop, then we should look a 
> >> little closer.
> >>
> >> Greg
> >>
> >>> On Sep 5, 2017, at 2:06 PM, Davide Italiano  wrote:
> >>>
> >>> On Tue, Sep 5, 2017 at 2:03 PM, Jason Molenda  wrote:
> 
> 
> > On Sep 5, 2017, at 1:34 PM, Davide Italiano  
> > wrote:
> >
> > On Tue, Sep 5, 2017 at 1:23 PM, Jason Molenda  
> > 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

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

[Zachary Turner via lldb-commits]

The C-string table and strcmp solution is fine, but I think the
StringSwitch method is strictly better. It's both faster (although I think
we agreed that speed doesn't matter in this case) //and// more readable.

Another alternative would be:

constexpr StringRef Registers[] = {"r12", "r13", "r14", "r15", "rbp",
"rbx", "ebp", "ebx", "rip", "eip", "rsp", "esp", "sp", "fp", "pc"};
bool IsCalleeSaved = llvm::is_contained(Registers, reg_info->name);

On Tue, Sep 5, 2017 at 3:30 PM Jason Molenda  wrote:

> I don't think anyone disagrees with that -- the simple c-string table &
> strcmp solution is fine.  It's fun to muse about different approaches like
> this, though.  More generally, I think the way lldb handles registers is
> one of the less well-designed parts of the debugger and it's something I
> often think about in the back of my mind, how it could possibly be done
> better than it is today.  You can see me experimenting with an idea with
> the RegisterNumber class I use in RegisterContextLLDB - this is one tiny
> part of the problem that I was trying to simplify in the unwinder.
>
>
> > On Sep 5, 2017, at 3:22 PM, Zachary Turner  wrote:
> >
> > Unless it's showing up on a profile, isn't all of this just a solution
> looking for a problem?  Davide claims neither the original or updated code
> shows up on any profiles, so why don't we just default to readable?
> >
> > On Tue, Sep 5, 2017 at 3:17 PM Greg Clayton  wrote:
> > We should actually populate the register info with a cached value of
> this so we do this once per process?
> >
> > > On Sep 5, 2017, at 3:12 PM, Jason Molenda  wrote:
> > >
> > > This method gets called every time we try to read a register in the
> unwinder when we're above stack frame 0.  The unwinder won't try to fetch
> volatile (non-callee-spilled) registers, and it uses this ABI method to
> check before trying to retrieve the reg.
> > >
> > > We could switch the preserved register name table to be ConstString's
> and pay a one-time encoding cost for them, but the 'struct RegisterInfo'
> stores its register name as a c-string so we'd need to encode that into a
> ConstString every time we enter the method.
> > >
> > > (or change RegisterInfo to have a ConstString rep of the register name
> instead of a c-string.  which wouldn't be a bad idea.)
> > >
> > >
> > >> On Sep 5, 2017, at 3:04 PM, Greg Clayton  wrote:
> > >>
> > >> You could also use a collection of lldb_private::ConstString objects.
> Comparing those are pointer compares since lldb_private::ConstString unique
> the strings into a string pool. lldb_private::UniqueCStringMap has a very
> efficient way to do this if you need an example.
> > >>
> > >> Not sure how many times we call this function. If it is once per
> target run, then who cares. If it is every time we stop, then we should
> look a little closer.
> > >>
> > >> Greg
> > >>
> > >>> On Sep 5, 2017, at 2:06 PM, Davide Italiano 
> wrote:
> > >>>
> > >>> On Tue, Sep 5, 2017 at 2:03 PM, Jason Molenda 
> wrote:
> > 
> > 
> > > On Sep 5, 2017, at 1:34 PM, Davide Italiano 
> wrote:
> > >
> > > On Tue, Sep 5, 2017 at 1:23 PM, Jason Molenda 
> 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 perf