Repository navigation
gh-161467: Enable __attribute__((hot)) for Clang - #161468
chenshanzhi wants to merge 4 commits into
Conversation
The history of compiler support for the relevant features is as follows. * GCC introduced `__attribute__((hot))` in GCC 4.3.0. * https://gcc.gnu.org/pipermail/gcc-patches/2017-July/478580.html * GCC has supported for `__has_attribute()` since GCC 5. * https://gcc.gnu.org/gcc-5/changes.html * Clang has supported `__attribute__((hot))` since Clang 3.2. * llvm/llvm-project@29c2b43 * Clang has supported `__has_attribute()` since Clang 2.9. * llvm/llvm-project@274a70e * By default, Clang defines `__GNUC__` as 4, `__GNUC_MINOR__` as 2, and `__GNUC_PATCHLEVEL__` as 1. * llvm/llvm-project#99804 Therefore, `__attribute__((hot))` is not enabled when building CPython with Clang's default configuration. And we can use `_Py__has_attribute(hot)` or `GCC version >= 4.3` to determine whether the hot attribute is supported, and enable this attribute for Clang and other compilers that support it.
|
All commit authors signed the Contributor License Agreement. |
|
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
|
Can you please show PyPerformance changes with this, both on a release build and on a PGO build? |
| #if _Py__has_attribute(hot) || defined(__GNUC__) \ | ||
| && ((__GNUC__ >= 5) || (__GNUC__ == 4) && (__GNUC_MINOR__ >= 3)) | ||
| #define _Py_HOT_FUNCTION __attribute__((hot)) | ||
| #else | ||
| #define _Py_HOT_FUNCTION | ||
| #endif |
There was a problem hiding this comment.
GCC 5 was releasesd 11 years, IMO it's ok to drop this optimization on GCC 5 which shouldn't be used in the wild anymore. So I suggest to simplify the code (and add indentation for readability):
| #if _Py__has_attribute(hot) | |
| # define _Py_HOT_FUNCTION __attribute__((hot)) | |
| #else | |
| # define _Py_HOT_FUNCTION | |
| #endif |
Note: I'm the one who added _Py_HOT_FUNCTION to Python a few years ago :-)
When using PGO, I'm not sure that
|
|
The comment above says:
We don't know how Clang will behave/optimise, so I think we should be cautious and verify there is no regression before merging. |
| * Usage: | ||
| * int _Py_HOT_FUNCTION x(void) { return 3; } | ||
| * | ||
| * Issue #28618: This attribute must not be abused, otherwise it can have a |
There was a problem hiding this comment.
While you're here, can you update the issue number to issue gh-72804 please? See: #72804
The history of compiler support for the relevant features is as follows.
__attribute__((hot))in GCC 4.3.0.__has_attribute()since GCC 5.__attribute__((hot))since Clang 3.2.__has_attribute()since Clang 2.9.__GNUC__as 4,__GNUC_MINOR__as 2, and__GNUC_PATCHLEVEL__as 1.__GNUC__macros llvm/llvm-project#99804Therefore,
__attribute__((hot))is not enabled when building CPython with Clang's default configuration. And we can use_Py__has_attribute(hot)orGCC version >= 4.3to determine whether the hot attribute is supported, and enable this attribute for Clang and other compilers that support it.