> On 17 Jun 2025, at 4:18 pm, Dhruv Chawla <dhr...@nvidia.com> wrote:
>
> On 17/06/25 06:10, Kugan Vivekanandarajah wrote:
>> External email: Use caution opening links or attachments
>> Hi,
>> As discusses earlier, get_original_name is used to match profile binary
>> names to
>> the symbol names in the IR during auto-profile pass.
>
> I think it could be good to add a link to the mailing list archive for this.
>
>> Hence, we want to strip the compiler added suffixes for names
>> that are generated after auto-profile pass.
>> Names we should strip:
>> * SRA clones (of the form foo.isra.0)
>> * IPA-CP clonse (in the form bar.constprop.123)
>> * Partial inlining clones (foo.part.0, foo.cold)
>> * lto_priv.0
>> * Nested function as foo.part.0
>> Others that should not be stripped:
>> * target_clones (foo.arch_x86_64_v2, foo.avx2)
>> * OpenMP related suffixes (.omp_fn.N, .omp_cpyfn.N)
>> Tested by running autoprofiledbootstrap and tree-prof check that
>> exercises auto-profile pass.
>> gcc/ChangeLog:
>> * auto-profile.cc (isAsciiDigit): New.
>> (get_original_name): Strip suffixes only for compiler generated
>> names tat happens after auto-profile.
>> Thanks,
>> Kugan
>> diff --git a/gcc/auto-profile.cc b/gcc/auto-profile.cc
>> index e12b3048f20..c787bf2d220 100644
>> --- a/gcc/auto-profile.cc
>> +++ b/gcc/auto-profile.cc
>> @@ -345,16 +345,50 @@ static gcov_summary *afdo_profile_info;
>> /* Helper functions. */
>> +bool isAsciiDigit (char c)
>> +{
>> + return c >= '0' && c <= '9';
>> +}
>
> Not sure about the naming conventions, and slight nit, sorry,
> but shouldn't this be snake case, like 'is_ascii_digit'? Also,
> it looks like GCC has an `ISDIGIT` macro, could that be used
> instead?
>
>> +
>> /* Return the original name of NAME: strip the suffix that starts
>> - with '.' Caller is responsible for freeing RET. */
>> + with '.' for names that are generetad after auto-profile pass.
>> + This is to match profiled names with the names in the IR at this stage.
>> + Caller is responsible for freeing RET. */
>> static char *
>> get_original_name (const char *name)
>> {
>> char *ret = xstrdup (name);
>> - char *find = strchr (ret, '.');
>> - if (find != NULL)
>> - *find = 0;
>> + char *last_dot = strrchr (ret, '.');
>> + if (last_dot == NULL)
>> + return ret;
>> + bool only_digits = true;
>> + char *ptr = last_dot;
>> + while (*(++ptr) != 0)
>> + if (!isAsciiDigit (*ptr))
>> + {
>> + only_digits = false;
>> + break;
>> + }
>> + if (only_digits)
>> + *last_dot = 0;
>> + char *next_dot = strrchr (ret, '.');
>> + /* if nested function such as foo.0, return foo. */
>> + if (next_dot == NULL)
>> + return ret;
>> + /* Suffixes of clones that compiler generates after auto-profile. */
>> + const char *suffixes[] = {"isra", "constprop", ".lto_priv", "part",
>> "cold"};
>
> ".lto_priv" should just be "lto_priv" here.
Yes, it is a typo. Let me fix this.
>
>> + for (unsigned i = 0; i < sizeof (suffixes); ++i)
>> + {
>> + if (strncmp (next_dot + 1, suffixes[i], strlen (suffixes[i])) == 0)
>> + {
>> + *next_dot = 0;
>> + return ret;
>> + }
>> + }
>> + /* Otherwise, it is for clones such as .omp_fn.N that was done before
>> + auto-profile and should be kept as it is. */
>> + *last_dot = '.';
>> return ret;
>> }
>
> It looks like this function only strips the first suffix? I don't think this
> would work for functions with multiple suffixes like
> "bar1.constprop.0.isra.0",
> where an IPA-CP clone has an SRA clone created for it. I haven't tested the
> patch, but I think it would return "bar1.constprop.0" for the above function
> name, whereas the current implementation returns "bar1" as expected.
Good point. I think we should do it recursively. Let me fix that.
Thanks,
Kugan
>
> --
> Regards,
> Dhruv
