bpo-44340: Add support for building with clang thin lto via --with-lto=thin by holmanb · Pull Request #26585 · python/cpython · GitHub

This adds support for building cpython with clang's --flto=thin option. Existing --with-lto behavior remains unchanged (default to no, with --with-lto currently using the default compiler lto option).

The tests (make test) currently pass for clang 11.1.0 for each of --with-lto and --with-lto=thin.

https://bugs.python.org/issue44340

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA).

CLA Missing

Our records indicate the following people have not signed the CLA:

@holmanb

For legal reasons we need all the people listed to sign the CLA before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

If you have recently signed the CLA, please wait at least one business day
before our records are updated.

You can check yourself to see if the CLA has been received.

Thanks again for the contribution, we look forward to reviewing it!

Thanks for the PR. Without commenting on the desirability of the feature, be aware thatconfigure is a derived file produced from configure.ac by the autoconf tool. The PR needs to change configure.ac and then run autoconf and commit the resulting changes to configure too. See the devguide for more info.

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

Please add the NEWS.d with blurb tool

@holmanb
I am a big fan of the thin LTO feature,
I will follow up on this PR. please ping me if you need my help cc @ned-deily

Let's run the https://github.com/python/pyperformance benchmark and compare it with full LTO
I would like to see how thin LTO executes aggressive optimization pass.
(Ref: http://blog.llvm.org/2016/06/thinlto-scalable-and-incremental-lto.html)

cc @vstinner @pablogsal @ambv

What about

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

@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

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

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

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

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

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.

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

@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?

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...

#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)

Thanks for the PR. Without commenting on the desirability of the feature, be aware thatconfigure is a derived file produced from configure.ac by the autoconf tool. The PR needs to change configure.ac and then run autoconf and commit the resulting changes to configure too. See the devguide for more info.

@ned-deily The current configure file in main branch is not the product of running autoconf with configure.ac. What is the best way to proceed? 2fc857a is one example of a change that didn't update configure.ac.

I assume that getting this fixed upstream is a prerequisite for getting this PR merged, otherwise autoconf will generate a configure that rips out some recent updates to configure.

Perhaps a CI bot to verify that in future PRs autoconf(configure.ac) -> configure would be appropriate?

the-knights-who-say-ni added the CLA not signed label Jun 7, 2021

bedevere-bot added the awaiting review label Jun 7, 2021

ned-deily requested changes Jun 8, 2021

View reviewed changes

bedevere-bot removed the awaiting review label Jun 8, 2021

bedevere-bot added the awaiting changes label Jun 8, 2021

add support for clang thin lto via --with-lto=thin

38de7cd

holmanb force-pushed the main branch from 8c0dfb9 to 38de7cd Compare June 9, 2021 03:47

the-knights-who-say-ni added CLA signed and removed CLA not signed labels Jul 8, 2021

corona10 closed this Jul 18, 2021

corona10 reopened this Jul 18, 2021

corona10 requested changes Jul 18, 2021

View reviewed changes

corona10 self-assigned this Jul 18, 2021

📜🤖 Added by blurb_it.

5a36cd4

blurb fixup

625321e

corona10 changed the title bpo-44340 Add support for building with clang thin lto via --with-lto=thin bpo-44340: Add support for building with clang thin lto via --with-lto=thin Jul 18, 2021

corona10 requested changes Jul 18, 2021

View reviewed changes

Comment thread Misc/NEWS.d/next/Build/2021-07-18-05-35-33.bpo-bpo-44340.1xHAZP.rst Outdated

corona10 reviewed Jul 18, 2021

View reviewed changes

Fix blurb file name and rst format.

9405a02

corona10 reviewed Jul 18, 2021

View reviewed changes

pablogsal reviewed Jul 18, 2021

View reviewed changes

corona10 closed this Jul 19, 2021

                                         (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

                 --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
                                         is no)

+                        LTOFLAGS="-flto$LTO_ARG -Wl,-export_dynamic"
+                        LTOCFLAGS="-flto$LTO_ARG"
                         ;;
                       *)
-                        LTOFLAGS="-flto"
+                        LTOFLAGS="-flto$LTO_ARG"

bpo-44340: Add support for building with clang thin lto via --with-lto=thin by holmanb · Pull Request #26585 · python/cpython · GitHub

Repository navigation

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

bpo-44340: Add support for building with clang thin lto via --with-lto=thin#26585holmanb wants to merge 4 commits intopython:mainpython/cpython:mainfrom holmanb:mainholmanb/cpython:mainCopy head branch name to clipboard

Conversation

holmanb commented Jun 7, 2021 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

the-knights-who-say-ni commented Jun 7, 2021

CLA Missing

Uh oh!

ned-deily left a comment

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Jun 8, 2021

Uh oh!

corona10 left a comment

Choose a reason for hiding this comment

Uh oh!

corona10 commented Jul 18, 2021

Uh oh!

corona10 commented Jul 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

corona10 Jul 18, 2021

Choose a reason for hiding this comment

Uh oh!

corona10 Jul 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pablogsal Jul 18, 2021

Choose a reason for hiding this comment

Uh oh!

holmanb Jul 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

corona10 Jul 18, 2021

Choose a reason for hiding this comment

Uh oh!

pablogsal Jul 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

holmanb Jul 18, 2021

Choose a reason for hiding this comment

Uh oh!

holmanb Jul 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pablogsal Jul 18, 2021

Choose a reason for hiding this comment

Uh oh!

corona10 Jul 18, 2021

Choose a reason for hiding this comment

Uh oh!

holmanb commented Jul 18, 2021

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

bpo-44340: Add support for building with clang thin lto via --with-lto=thin#26585
holmanb wants to merge 4 commits into
python:mainfrom
holmanb:main

holmanb commented Jun 7, 2021 •
edited by bedevere-bot
Loading

corona10 commented Jul 18, 2021 •
edited
Loading

corona10 Jul 18, 2021 •
edited
Loading

holmanb Jul 18, 2021 •
edited
Loading

pablogsal Jul 18, 2021 •
edited
Loading

holmanb Jul 18, 2021 •
edited
Loading