Hi, Thanks for looking. > > I think this can be simplified to > > while (*(++ptr) && only_digits) > only_digits = is_digit (*ptr); IMO this is harder to read but I am OK either way.
>
>> + 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"};
>> + for (unsigned i = 0; i < sizeof (suffixes); ++i)
>
> sizeof (suffixes) seems like a bug here. This will return 40 instead of 5 on a
> 64-bit system, and I think this should be (sizeof (suffixes) / sizeof
> (*suffixes)).
Changed it to sizeof (suffixes) / sizeof (suffixes[0])
>
>> + {
>> + if (strncmp (next_dot + 1, suffixes[i], strlen (suffixes[i])) == 0)
>> + {
>> + *next_dot = 0;
>> + return get_original_name (ret, false);
>
> Given that this is tail-recursive, I feel like recursion is not necessary here
> and it would be more efficient to have this be a loop instead. The
> implementation looks okay as is, though.
IMO doing this in a loop would have to handle all the above cases and would
make it hard to read. Also, we would have two level for now. Even if this
change in the future, this is not going to be too long.
Here is the revised patch,
Is this OK for mainline.
Thanks,
Kugan
>
>> + }
>> + }
>> + /* 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;
>> }
>
> --
> Regards,
> Dhruv
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
