On 18/06/25 14:21, Kugan Vivekanandarajah wrote:
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
diff --git a/gcc/auto-profile.cc b/gcc/auto-profile.cc
index 8918d1e9f09..0fb29aad5d3 100644
--- a/gcc/auto-profile.cc
+++ b/gcc/auto-profile.cc
@@ -345,16 +345,51 @@ static gcov_summary *afdo_profile_info;
/* Helper functions. */
+static bool is_digit (char c)
+{
+ return c >= '0' && c <= '9';
+}
+
/* 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.
+ Note that we only have to strip sufix and not in the middle.
+ Caller is responsible for freeing RET. */
static char *
-get_original_name (const char *name)
+get_original_name (const char *name, bool alloc = true)
{
- char *ret = xstrdup (name);
- char *find = strchr (ret, '.');
- if (find != NULL)
- *find = 0;
+ char *ret = alloc ? xstrdup (name) : const_cast<char *> (name);
+ char *last_dot = strrchr (ret, '.');
+ if (last_dot == NULL)
+ return ret;
+ bool only_digits = true;
+ char *ptr = last_dot;
+ while (*(++ptr) != 0)
+ if (!is_digit (*ptr))
+ {
+ only_digits = false;
+ break;
+ }
I think this can be simplified to
while (*(++ptr) && only_digits)
only_digits = is_digit (*ptr);
+ 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)).
+ {
+ 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.
+ }
+ }
+ /* 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
