Re: [PATCH 2/2] gdb, amd-dbgapi-target: split wave_coordinates::to_string

[Simon Marchi <simark@simark.ca>]

On 7/3/26 8:50 AM, Tankut Baris Aktemur wrote:
> wave_coordinates::to_string() produces a string in the following format:
> 
>   AMDGPU Wave a:q:d:w (x,y,z)/i
> 
> Split the method into smaller pieces:
> 
>   a:q:d:w   : hierarchy_str
>   (x,y,z)   : workgroup_coord_str
>   (x,y,z)/i : dispatch_pos_str
> 
> This is a refactoring to allow reusing pieces.  Currently the reuse
> opportunity exists in the downstream debugger.

I think this is a nice change even for upstream, as it helps document
what the numbers mean.  Assuming Lancelot is already fine with the
change:

Approved-By: Simon Marchi <simon.marchi@efficios.com>

Just one formatting comment below.

> ---
>  gdb/amd-dbgapi-target.c | 55 +++++++++++++++++++++++++++++++++--------
>  1 file changed, 45 insertions(+), 10 deletions(-)
> 
> diff --git a/gdb/amd-dbgapi-target.c b/gdb/amd-dbgapi-target.c
> index 923b790664e..33512695fb5 100644
> --- a/gdb/amd-dbgapi-target.c
> +++ b/gdb/amd-dbgapi-target.c
> @@ -147,6 +147,17 @@ struct wave_coordinates
>      : wave_id (wave_id)
>    {}
>  
> +  /* Return the string showing the agent -> queue -> dispatch -> wave
> +     hierarchy.  */
> +  std::string hierarchy_str () const;
> +
> +  /* Return the workgroup coordinates as a string.  */
> +  std::string workgroup_coord_str () const;
> +
> +  /* Return the dispatch position string for the wave this
> +     wave_coordinates is for.  */
> +  std::string dispatch_pos_str () const;
> +
>    /* Return the target ID string for the wave this wave_coordinates is
>       for.  */
>    std::string to_string () const;
> @@ -387,13 +398,12 @@ get_amd_dbgapi_inferior_info (inferior *inferior)
>  static async_event_handler *amd_dbgapi_async_event_handler = nullptr;
>  
>  std::string
> -wave_coordinates::to_string () const
> +wave_coordinates::hierarchy_str () const
>  {
> -  std::string str = "AMDGPU Wave";
> -
> -  str += (agent_id != AMD_DBGAPI_AGENT_NONE
> -	  ? string_printf (" %s", pulongest (agent_id.handle))
> -	  : " ?");
> +  std::string str
> +    = (agent_id != AMD_DBGAPI_AGENT_NONE
> +       ? string_printf (" %s", pulongest (agent_id.handle))
> +       : "?");
>  
>    str += (queue_id != AMD_DBGAPI_QUEUE_NONE
>  	  ? string_printf (":%s", pulongest (queue_id.handle))
> @@ -405,11 +415,25 @@ wave_coordinates::to_string () const
>  
>    str += string_printf (":%s", pulongest (wave_id.handle));
>  
> -  str += (group_ids[0] != UINT32_MAX
> -	  ? string_printf (" (%u,%u,%u)", group_ids[0], group_ids[1],
> -			   group_ids[2])
> -	  : " (?,?,?)");
> +  return str;
> +}
> +
> +std::string
> +wave_coordinates::workgroup_coord_str () const
> +{
> +  std::string str
> +    = (group_ids[0] != UINT32_MAX
> +       ? string_printf ("(%u,%u,%u)", group_ids[0], group_ids[1],
> +			group_ids[2])

group_ids[2] could go on the previous line, for one less line wrap.

Simon