GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-44340: Add support for building with clang thin lto via --with-lto=thin by holmanb · Pull Request #26585 · python/cpython

Conversation

@holmanb

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

Uh oh!

There was an error while loading. Please reload this page.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@the-knights-who-say-ni

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

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!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@the-knights-who-say-ni the-knights-who-say-ni added the CLA not signed label Jun 7, 2021
@bedevere-bot bedevere-bot added the awaiting review label Jun 7, 2021
ned-deily
ned-deily requested changes Jun 8, 2021
View reviewed changes

@ned-deily ned-deily left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-bot bedevere-bot removed the awaiting review label Jun 8, 2021
@bedevere-bot

bedevere-bot commented Jun 8, 2021

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot added the awaiting changes label Jun 8, 2021
@holmanb
add support for clang thin lto via --with-lto=thin
38de7cd
@holmanb
holmanb force-pushed the main branch from 8c0dfb9 to 38de7cd Compare June 9, 2021 03:47
@the-knights-who-say-ni the-knights-who-say-ni added CLA signed and removed CLA not signed labels Jul 8, 2021
@corona10 corona10 closed this Jul 18, 2021
@corona10 corona10 reopened this Jul 18, 2021
corona10
corona10 requested changes Jul 18, 2021
View reviewed changes

@corona10 corona10 left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Please add the NEWS.d with blurb tool

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@corona10

corona10 commented Jul 18, 2021

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@corona10 corona10 self-assigned this Jul 18, 2021
@blurb-it
📜🤖 Added by blurb_it.
5a36cd4
@corona10

corona10 commented Jul 18, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@holmanb
blurb fixup
625321e
@corona10 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
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 Resolved

Uh oh!

There was an error while loading. Please reload this page.
corona10
corona10 reviewed Jul 18, 2021
View reviewed changes
Comment thread configure
(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

@corona10 corona10 Jul 18, 2021

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@holmanb
Fix blurb file name and rst format.
9405a02
corona10
corona10 reviewed Jul 18, 2021
View reviewed changes
Comment thread configure
--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)

@corona10 corona10 Jul 18, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
pablogsal
pablogsal reviewed Jul 18, 2021
View reviewed changes
Comment thread configure.ac
Comment on lines +1415 to +1419
LTOFLAGS="-flto$LTO_ARG -Wl,-export_dynamic"
LTOCFLAGS="-flto$LTO_ARG"
;;
*)
LTOFLAGS="-flto"
LTOFLAGS="-flto$LTO_ARG"

@pablogsal pablogsal Jul 18, 2021

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@holmanb holmanb Jul 18, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@corona10 corona10 Jul 18, 2021

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@pablogsal pablogsal Jul 18, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@holmanb holmanb Jul 18, 2021

Copy link
Copy Markdown
Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@holmanb holmanb Jul 18, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Author

There was a problem hiding this comment.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
corona10 and pablogsal reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

@pablogsal pablogsal Jul 18, 2021

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@corona10 corona10 Jul 18, 2021

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@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?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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

Reviewers

@corona10 corona10 corona10 requested changes
@pablogsal pablogsal pablogsal left review comments
@ned-deily ned-deily ned-deily requested changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@corona10 corona10

Labels

awaiting changes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@holmanb @the-knights-who-say-ni @bedevere-bot @corona10 @ned-deily @pablogsal
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.