iframe-proxy

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

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

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

                          (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

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

+          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

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

Sunbelt Computer Software

PL/B Language Development and Support

Uh oh!

Uh oh!

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

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

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