Skip to content

bpo-44340: Add support for building with clang thin lto via --with-lto=thin #26585

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
holmanb wants to merge 4 commits into from
Closed

bpo-44340: Add support for building with clang thin lto via --with-lto=thin #26585

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
38de7cd
add support for clang thin lto via --with-lto=thin
holmanb Jun 7, 2021
5a36cd4
📜🤖 Added by blurb_it.
blurb-it[bot] Jul 18, 2021
625321e
blurb fixup
holmanb Jul 18, 2021
9405a02
Fix blurb file name and rst format.
holmanb Jul 18, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions Misc/NEWS.d/next/Build/2021-07-18-05-35-33.bpo-44340.1xHAZP.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Add support for building with clang's "thin" link time optimization using
``--with-lto=thin``.
33 changes: 20 additions & 13 deletions configure
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1545,7 +1545,7 @@ Optional Packages:
--with-trace-refs enable tracing references for debugging purpose
(default is no)
--with-assertions build with C assertions enabled (default is no)
--with-lto enable Link-Time-Optimization in any build (default
--with-lto[=no|thin] enable Link-Time-Optimization in any build (default
Copy link
Member

@corona10 corona10 Jul 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about

--with-lto[=full|thin] enable Link-Time-Optimization in any build (default is full)

is no)
Copy link
Member

@corona10 corona10 Jul 18, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@holmanb

the current no means that whether we apply LTO or not.
So the configure should be updated if we also provide the thin LTO option.

Following options should be available.
A. no LTO (by default) ./configure
B. with LTO (default=full) ./configure --with-lto
C. with LTO (full LTO designated) ./configure --with-lto=full
D. with LTO (thin LTO designated) ./configure --with-lto=thin

--with-hash-algorithm=[fnv|siphash24]
select hash algorithm for use in Python/pyhash.c
Expand Down Expand Up @@ -6585,16 +6585,23 @@ $as_echo_n "checking for --with-lto... " >&6; }
# Check whether --with-lto was given.
if test "${with_lto+set}" = set; then :
withval=$with_lto;
if test "$withval" != no
then
Py_LTO='true'
{ $as_echo "$as_me:${as_lineno-$LINENO}: result: yes" >&5
$as_echo "yes" >&6; };
else
Py_LTO='false'
{ $as_echo "$as_me:${as_lineno-$LINENO}: result: no" >&5
LTO_ARG=''
if test "$withval" = no
then
Py_LTO='false'
{ $as_echo "$as_me:${as_lineno-$LINENO}: result: no" >&5
$as_echo "no" >&6; };
fi
elif test "$withval" = yes
then
Py_LTO='true'
{ $as_echo "$as_me:${as_lineno-$LINENO}: result: yes" >&5
$as_echo "yes" >&6; };
else
Py_LTO='true'
{ $as_echo "$as_me:${as_lineno-$LINENO}: result: \"$withval\"" >&5
$as_echo "\"$withval\"" >&6; };
LTO_ARG="=$withval"
fi
else
{ $as_echo "$as_me:${as_lineno-$LINENO}: result: no" >&5
$as_echo "no" >&6; }
Expand Down Expand Up @@ -6732,11 +6739,11 @@ $as_echo "$as_me: llvm-ar found via xcrun: ${LLVM_AR}" >&6;}
case $ac_sys_system in
Darwin*)
# Any changes made here should be reflected in the GCC+Darwin case below
LTOFLAGS="-flto -Wl,-export_dynamic"
LTOCFLAGS="-flto"
LTOFLAGS="-flto$LTO_ARG -Wl,-export_dynamic"
LTOCFLAGS="-flto$LTO_ARG"
;;
*)
LTOFLAGS="-flto"
LTOFLAGS="-flto$LTO_ARG"
;;
esac
;;
Expand Down
31 changes: 19 additions & 12 deletions configure.ac
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1360,16 +1360,23 @@ fi

# Enable LTO flags
AC_MSG_CHECKING(for --with-lto)
AC_ARG_WITH(lto, AS_HELP_STRING([--with-lto], [enable Link-Time-Optimization in any build (default is no)]),
AC_ARG_WITH(lto, AS_HELP_STRING([--with-lto@<:@=no|thin@:>@], [enable Link-Time-Optimization in any build (default is no)]),

[
if test "$withval" != no
then
Py_LTO='true'
AC_MSG_RESULT(yes);
else
Py_LTO='false'
AC_MSG_RESULT(no);
fi],
LTO_ARG=''
if test "$withval" = no
then
Py_LTO='false'
AC_MSG_RESULT(no);
elif test "$withval" = yes
then
Py_LTO='true'
AC_MSG_RESULT(yes);
else
Py_LTO='true'
AC_MSG_RESULT("$withval");
LTO_ARG="=$withval"
fi],
[AC_MSG_RESULT(no)])
if test "$Py_LTO" = 'true' ; then
case $CC in
Expand Down Expand Up @@ -1405,11 +1412,11 @@ if test "$Py_LTO" = 'true' ; then
case $ac_sys_system in
Darwin*)
# Any changes made here should be reflected in the GCC+Darwin case below
LTOFLAGS="-flto -Wl,-export_dynamic"
LTOCFLAGS="-flto"
LTOFLAGS="-flto$LTO_ARG -Wl,-export_dynamic"
LTOCFLAGS="-flto$LTO_ARG"
;;
*)
LTOFLAGS="-flto"
LTOFLAGS="-flto$LTO_ARG"
Comment on lines +1415 to +1419
Copy link
Member

@pablogsal pablogsal Jul 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are the flags (thing or otherwise) supported by all compilers that support LTO?

Copy link
Author

@holmanb holmanb Jul 18, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. Thinlto is a more recent implementation than the default that is specific to clang.

Copy link
Member

@corona10 corona10 Jul 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/python/cpython/blob/55a4147b16a1f96567779218f71d2ab8507ff220/configure.ac#L1362#L1460

Here's something I think about supporting the LTO options.

Copy link
Member

@pablogsal pablogsal Jul 18, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then we should probably need show an error if the compiler doesn't support it or/and document the limitation, because otherwise, it seems that is a general option that we are exposing regardless of the compiler.

Copy link
Author

@holmanb holmanb Jul 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@corona10 - I like it. That's more readable and provides a warning as @pablogsal suggested.

Copy link
Author

@holmanb holmanb Jul 18, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@corona10 Nitpick: gcc doesn't have a concept of full, and if --with-lto=full is configured with gcc, no warnings are thrown and -flto is assumed. Is this assumption sensible? Or would throwing an error on a nonsense input be better than assuming intent?

Copy link
Member

@pablogsal pablogsal Jul 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think given that these options are only available on clang we should just fail if the compiler is not clang. Adding checks to other compilers one by one is going to be suboptimal because CPython can be compiled with almost any C compiler: xcl, icc...

Copy link
Member

@corona10 corona10 Jul 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#27231

Let's discuss the supporting option with the attached PR.

  • We may check clang version
  • We may check other compiler support (clang, gcc supports LTO itself, others not)

;;
esac
;;
Expand Down