diff mbox series

[v3] RISC-V: Avoid updating state until symbol is found

Message ID 20231130035520.1369012-1-patrick@rivosinc.com
State Unresolved
Headers
Series [v3] RISC-V: Avoid updating state until symbol is found |

Checks

Context Check Description
snail/binutils-gdb-check warning Git am fail log

Commit Message

Patrick O'Neill Nov. 30, 2023, 3:55 a.m. UTC 
  Currently objdump gets and updates the map state once per symbol. Updating the
state (partiularly riscv_parse_subset) is expensive and grows quadratically
since we iterate over all symbols. By deferring this until once we've found the
symbol of interest, we can reduce the time to dump a 4k insn file of .norvc and
.rvc insns from ~47 seconds to ~0.13 seconds.

opcodes/ChangeLog:

	* riscv-dis.c (riscv_get_map_state): Remove state updating logic
	and rename to riscv_is_valid_mapping_symbol.
	(riscv_update_map_state): Add state updating logic to seperate function.
	(riscv_search_mapping_symbol): Use new riscv_update_map_state.
	(riscv_data_length): Ditto.

Signed-off-by: Patrick O'Neill <patrick@rivosinc.com>
---
Re-tested using rv64gcv make report-linux and make report-binutils-linux.
Binutils hash used/applied to: c618a1c548193d2a6a8c3d909a3d1c620a156b5d
GCC hash used: eecdd96c8d1de244e21212a830e51062b3e444c5

v3 changelog:
- Move and unify riscv_update_map_state calls into if (found) block

v2 changelog:
- Remove unneeded riscv_update_state calls on !found
- Rename riscv_get_map_state -> riscv_is_valid_mapping_symbol
---
 opcodes/riscv-dis.c | 43 +++++++++++++++++++++++++++++++------------
 1 file changed, 31 insertions(+), 12 deletions(-)

Comments

Nelson Chu Nov. 30, 2023, 7:56 a.m. UTC | #1 
Thanks for helping this, looks good to me, so committed.

Nelson

On Thu, Nov 30, 2023 at 11:55 AM Patrick O'Neill <patrick@rivosinc.com>
wrote:

> Currently objdump gets and updates the map state once per symbol. Updating
> the
> state (partiularly riscv_parse_subset) is expensive and grows quadratically
> since we iterate over all symbols. By deferring this until once we've
> found the
> symbol of interest, we can reduce the time to dump a 4k insn file of
> .norvc and
> .rvc insns from ~47 seconds to ~0.13 seconds.
>
> opcodes/ChangeLog:
>
>         * riscv-dis.c (riscv_get_map_state): Remove state updating logic
>         and rename to riscv_is_valid_mapping_symbol.
>         (riscv_update_map_state): Add state updating logic to seperate
> function.
>         (riscv_search_mapping_symbol): Use new riscv_update_map_state.
>         (riscv_data_length): Ditto.
>
> Signed-off-by: Patrick O'Neill <patrick@rivosinc.com>
> ---
> Re-tested using rv64gcv make report-linux and make report-binutils-linux.
> Binutils hash used/applied to: c618a1c548193d2a6a8c3d909a3d1c620a156b5d
> GCC hash used: eecdd96c8d1de244e21212a830e51062b3e444c5
>
> v3 changelog:
> - Move and unify riscv_update_map_state calls into if (found) block
>
> v2 changelog:
> - Remove unneeded riscv_update_state calls on !found
> - Rename riscv_get_map_state -> riscv_is_valid_mapping_symbol
> ---
>  opcodes/riscv-dis.c | 43 +++++++++++++++++++++++++++++++------------
>  1 file changed, 31 insertions(+), 12 deletions(-)
>
> diff --git a/opcodes/riscv-dis.c b/opcodes/riscv-dis.c
> index 6fa9855e3cd..f7f4c0750ed 100644
> --- a/opcodes/riscv-dis.c
> +++ b/opcodes/riscv-dis.c
> @@ -873,20 +873,20 @@ riscv_disassemble_insn (bfd_vma memaddr,
>    return insnlen;
>  }
>
> -/* Return true if we find the suitable mapping symbol,
> -   and also update the STATE.  Otherwise, return false.  */
> +/* If we find the suitable mapping symbol update the STATE.
> +   Otherwise, do nothing.  */
>
> -static bool
> -riscv_get_map_state (int n,
> -                    enum riscv_seg_mstate *state,
> -                    struct disassemble_info *info)
> +static void
> +riscv_update_map_state (int n,
> +                       enum riscv_seg_mstate *state,
> +                       struct disassemble_info *info)
>  {
>    const char *name;
>
>    /* If the symbol is in a different section, ignore it.  */
>    if (info->section != NULL
>        && info->section != info->symtab[n]->section)
> -    return false;
> +    return;
>
>    name = bfd_asymbol_name(info->symtab[n]);
>    if (strcmp (name, "$x") == 0)
> @@ -913,10 +913,26 @@ riscv_get_map_state (int n,
>        else
>         riscv_parse_subset (&riscv_rps_dis, name + 2);
>      }
> -  else
> +}
> +
> +/* Return true if we find the suitable mapping symbol.
> +   Otherwise, return false.  */
> +
> +static bool
> +riscv_is_valid_mapping_symbol (int n,
> +                            struct disassemble_info *info)
> +{
> +  const char *name;
> +
> +  /* If the symbol is in a different section, ignore it.  */
> +  if (info->section != NULL
> +      && info->section != info->symtab[n]->section)
>      return false;
>
> -  return true;
> +  name = bfd_asymbol_name(info->symtab[n]);
> +  return (strcmp (name, "$x") == 0
> +         || strcmp (name, "$d") == 0
> +         || strncmp (name, "$xrv", 4) == 0);
>  }
>
>  /* Check the sorted symbol table (sorted by the symbol value), find the
> @@ -975,7 +991,7 @@ riscv_search_mapping_symbol (bfd_vma memaddr,
>        /* We have searched all possible symbols in the range.  */
>        if (addr > memaddr)
>         break;
> -      if (riscv_get_map_state (n, &mstate, info))
> +      if (riscv_is_valid_mapping_symbol (n, info))
>         {
>           symbol = n;
>           found = true;
> @@ -1002,7 +1018,7 @@ riscv_search_mapping_symbol (bfd_vma memaddr,
>           if (addr < (info->section ? info->section->vma : 0))
>             break;
>           /* Stop searching once we find the closed mapping symbol.  */
> -         if (riscv_get_map_state (n, &mstate, info))
> +         if (riscv_is_valid_mapping_symbol (n, info))
>             {
>               symbol = n;
>               found = true;
> @@ -1013,6 +1029,8 @@ riscv_search_mapping_symbol (bfd_vma memaddr,
>
>    if (found)
>      {
> +      riscv_update_map_state (symbol, &mstate, info);
> +
>        /* Find the next mapping symbol to determine the boundary of this
> mapping
>          symbol.  */
>
> @@ -1068,11 +1086,12 @@ riscv_data_length (bfd_vma memaddr,
>         {
>           bfd_vma addr = bfd_asymbol_value (info->symtab[n]);
>           if (addr > memaddr
> -             && riscv_get_map_state (n, &m, info))
> +             && riscv_is_valid_mapping_symbol (n, info))
>             {
>               if (addr - memaddr < length)
>                 length = addr - memaddr;
>               found = true;
> +             riscv_update_map_state (n, &m, info);
>               break;
>             }
>         }
> --
> 2.34.1
>
>
diff mbox series

Patch

diff --git a/opcodes/riscv-dis.c b/opcodes/riscv-dis.c
index 6fa9855e3cd..f7f4c0750ed 100644
--- a/opcodes/riscv-dis.c
+++ b/opcodes/riscv-dis.c
@@ -873,20 +873,20 @@  riscv_disassemble_insn (bfd_vma memaddr,
   return insnlen;
 }
 
-/* Return true if we find the suitable mapping symbol,
-   and also update the STATE.  Otherwise, return false.  */
+/* If we find the suitable mapping symbol update the STATE.
+   Otherwise, do nothing.  */
 
-static bool
-riscv_get_map_state (int n,
-		     enum riscv_seg_mstate *state,
-		     struct disassemble_info *info)
+static void
+riscv_update_map_state (int n,
+			enum riscv_seg_mstate *state,
+			struct disassemble_info *info)
 {
   const char *name;
 
   /* If the symbol is in a different section, ignore it.  */
   if (info->section != NULL
       && info->section != info->symtab[n]->section)
-    return false;
+    return;
 
   name = bfd_asymbol_name(info->symtab[n]);
   if (strcmp (name, "$x") == 0)
@@ -913,10 +913,26 @@  riscv_get_map_state (int n,
       else
 	riscv_parse_subset (&riscv_rps_dis, name + 2);
     }
-  else
+}
+
+/* Return true if we find the suitable mapping symbol.
+   Otherwise, return false.  */
+
+static bool
+riscv_is_valid_mapping_symbol (int n,
+			     struct disassemble_info *info)
+{
+  const char *name;
+
+  /* If the symbol is in a different section, ignore it.  */
+  if (info->section != NULL
+      && info->section != info->symtab[n]->section)
     return false;
 
-  return true;
+  name = bfd_asymbol_name(info->symtab[n]);
+  return (strcmp (name, "$x") == 0
+	  || strcmp (name, "$d") == 0
+	  || strncmp (name, "$xrv", 4) == 0);
 }
 
 /* Check the sorted symbol table (sorted by the symbol value), find the
@@ -975,7 +991,7 @@  riscv_search_mapping_symbol (bfd_vma memaddr,
       /* We have searched all possible symbols in the range.  */
       if (addr > memaddr)
 	break;
-      if (riscv_get_map_state (n, &mstate, info))
+      if (riscv_is_valid_mapping_symbol (n, info))
 	{
 	  symbol = n;
 	  found = true;
@@ -1002,7 +1018,7 @@  riscv_search_mapping_symbol (bfd_vma memaddr,
 	  if (addr < (info->section ? info->section->vma : 0))
 	    break;
 	  /* Stop searching once we find the closed mapping symbol.  */
-	  if (riscv_get_map_state (n, &mstate, info))
+	  if (riscv_is_valid_mapping_symbol (n, info))
 	    {
 	      symbol = n;
 	      found = true;
@@ -1013,6 +1029,8 @@  riscv_search_mapping_symbol (bfd_vma memaddr,
 
   if (found)
     {
+      riscv_update_map_state (symbol, &mstate, info);
+
       /* Find the next mapping symbol to determine the boundary of this mapping
 	 symbol.  */
 
@@ -1068,11 +1086,12 @@  riscv_data_length (bfd_vma memaddr,
 	{
 	  bfd_vma addr = bfd_asymbol_value (info->symtab[n]);
 	  if (addr > memaddr
-	      && riscv_get_map_state (n, &m, info))
+	      && riscv_is_valid_mapping_symbol (n, info))
 	    {
 	      if (addr - memaddr < length)
 		length = addr - memaddr;
 	      found = true;
+	      riscv_update_map_state (n, &m, info);
 	      break;
 	    }
 	}