◐ Shell
reader mode source ↗
Skip to content

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

Closed
holmanb wants to merge 4 commits into
python:mainfrom
holmanb:main
Closed

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

Conversation

@holmanb

@holmanb holmanb commented Jun 7, 2021

Copy link
Copy Markdown

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

Copy link
Copy Markdown

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 ned-deily left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hide comment

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

Copy link
Copy Markdown

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 corona10 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hide comment

Please add the NEWS.d with blurb tool

@corona10

Copy link
Copy Markdown
Member

@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 corona10 self-assigned this Jul 18, 2021
@corona10

corona10 commented Jul 18, 2021

Copy link
Copy Markdown
Member

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 corona10 changed the title bpo-44340 Add support for building with clang thin lto via --with-lto=thin Jul 18, 2021
@holmanb

holmanb commented Jul 18, 2021

Copy link
Copy Markdown
Author

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?

@corona10 corona10 closed this Jul 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants