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

holmanb · 2021-06-07T22:47:47Z

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

the-knights-who-say-ni · 2021-06-07T22:47:50Z

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!

ned-deily

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.

bedevere-bot · 2021-06-08T23:18:02Z

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.

corona10

Please add the NEWS.d with blurb tool

corona10 · 2021-07-18T04:50:32Z

@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

corona10 · 2021-07-18T05:40:44Z

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

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

corona10 · 2021-07-18T05:48:25Z

configure

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


What about

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

corona10 · 2021-07-18T06:12:38Z

configure

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


@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

pablogsal · 2021-07-18T15:52:46Z

configure.ac

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


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)

holmanb · 2021-07-18T16:59:45Z

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

Misc/NEWS.d/next/Build/2021-07-18-05-35-33.bpo-bpo-44340.1xHAZP.rst Outdated Show resolved Hide resolved

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

holmanb commented Jun 7, 2021 •

edited by bedevere-bot

Loading

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

ned-deily left a comment

bedevere-bot commented Jun 8, 2021

corona10 left a comment

corona10 commented Jul 18, 2021

corona10 commented Jul 18, 2021 •

edited

Loading

corona10 Jul 18, 2021

corona10 Jul 18, 2021 •

edited

Loading

pablogsal Jul 18, 2021

holmanb Jul 18, 2021 •

edited

Loading

corona10 Jul 18, 2021

pablogsal Jul 18, 2021 •

edited

Loading

holmanb Jul 18, 2021

holmanb Jul 18, 2021 •

edited

Loading

pablogsal Jul 18, 2021

corona10 Jul 18, 2021

holmanb commented Jul 18, 2021

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

Conversation

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

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

CLA Missing

ned-deily left a comment

Choose a reason for hiding this comment

bedevere-bot commented Jun 8, 2021

corona10 left a comment

Choose a reason for hiding this comment

corona10 commented Jul 18, 2021

corona10 commented Jul 18, 2021 • edited Loading

corona10 Jul 18, 2021

Choose a reason for hiding this comment

corona10 Jul 18, 2021 • edited Loading

Choose a reason for hiding this comment

pablogsal Jul 18, 2021

Choose a reason for hiding this comment

holmanb Jul 18, 2021 • edited Loading

Choose a reason for hiding this comment

corona10 Jul 18, 2021

Choose a reason for hiding this comment

pablogsal Jul 18, 2021 • edited Loading

Choose a reason for hiding this comment

holmanb Jul 18, 2021

Choose a reason for hiding this comment

holmanb Jul 18, 2021 • edited Loading

Choose a reason for hiding this comment

pablogsal Jul 18, 2021

Choose a reason for hiding this comment

corona10 Jul 18, 2021

Choose a reason for hiding this comment

holmanb commented Jul 18, 2021

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