Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable IPO / LTO and -O3 #8146

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from
Open

Enable IPO / LTO and -O3 #8146

wants to merge 7 commits into from

Conversation

rurban
Copy link
Contributor

@rurban rurban commented Dec 21, 2023

  • Each commit message has a non-empty body, explaining why the change was made.
  • Methods or procedures I have added are documented, following the guidelines provided in CODING_STANDARD.md.
  • The feature or user visible behaviour I have added or modified has been documented in the User Guide in doc/cprover-manual/
  • Regression or unit tests are included, or existing tests cover the modified code (in this case I have detailed which ones those are in the commit message).
  • My commit message includes data points confirming performance improvements (if claimed).
  • My PR is restricted to a single feature or bugfix.
  • White-space or formatting changes outside the feature-related changed lines are in commits of their own.

#define YYEMPTY -2
#define YYEOF 0 /* "end of file" */
#define YYerror 256 /* error */
#define YYUNDEF 257 /* "invalid token" */
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you please elaborate on what exactly was being warned about here?

%token TOK_VOLATILE "volatile"
%token TOK_WCHAR_T "wchar_t"
%token TOK_WHILE "while"
%token ATOK_AUTO "auto"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you please elaborate as to why token names that ought to be local to a translation unit need to be renamed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a -Wodr

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like %define api.token.prefix {prefix} would be the less-hacky solution, but that wouldn't work on macOS, which has bison < 3.0. So we will have to bite the bullet, thank you for coming up with this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like %define api.token.prefix {prefix} would be the less-hacky solution, but that wouldn't work on macOS, which has bison < 3.0. So we will have to bite the bullet, thank you for coming up with this.

The MacOS jobs in the CI pipeline are currently installing Bison version 3.8.2 from brew. There are commits following an api.token.prefix approach in this PR - #7878

@tautschnig
Copy link
Collaborator

https://lists.gnu.org/archive/html/bug-bison/2023-12/msg00001.html looks related, but hasn't seen a response as far as I can tell.

@rurban rurban force-pushed the lto branch 4 times, most recently from 998c03d to 2a37324 Compare October 31, 2024 12:08
@rurban rurban requested a review from qinheping as a code owner October 31, 2024 12:08
@rurban rurban force-pushed the lto branch 4 times, most recently from 472fbfa to 7093b70 Compare November 1, 2024 15:13
rurban and others added 6 commits November 17, 2024 19:02
needed when doxygen is found
LTO and -O3 give a good performance improvement.
I would not recommend -O3 with bad compiler versions though, such
as gcc-9.

WIP: GLOBAL properties are not enough, add the IPO property to all
targets. Which leads to various problems still.
See the next commit to fix yyalloc name collisions.
This is for CMake only as we haven't currently got an LTO set-up for
Makefile builds.
to avoid LTO -Wodr warnings. yacc and flex don't
support -P for these yet.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants