Hi,
> On 17 Jun 2025, at 4:51 pm, Kugan Vivekanandarajah <kvivekana...@nvidia.com>
> wrote:
>
> External email: Use caution opening links or attachments
>
>
>> 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.Here is the revised patch. Is this OK for master? Thanks Kugan
0001-AutoFDO-v2-Fix-get_original_name-to-strip-only-names.patch
Description: 0001-AutoFDO-v2-Fix-get_original_name-to-strip-only-names.patch
