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

Removing last remnants of pragma block at the top of pybind11.h #3186

Merged
merged 17 commits into from
Aug 14, 2021

Conversation

rwgk
Copy link
Collaborator

@rwgk rwgk commented Aug 9, 2021

Defaulting CUDA, GCC7, GCC8 to PYBIND11_NOINLINE_DISABLED, with the option to define PYBIND11_NOINLINE_FORCED. Relevant inline/noinline shared-library size differences are tabulated here. The measured size saving when using noinline are only 1.7% for CUDA, -0.2% for GCC7, and 0.0% for GCC8 (using -DCMAKE_BUILD_TYPE=MinSizeRel, the default under pybind11/tests).

Follow-on to PR #3127, based on results obtained under PR #3125.

Worksheet


This PR changes 2 files, removes 9 lines, adds 11 lines (6 of which are for a comment).


Suggested changelog entry:

The warnings-suppression "pragma clamp" at the top/bottom of pybind11 was removed, clearing the path to refactoring and IWYU cleanup.

CUDA, GCC7, GCC8 builds that prioritize noinline over -Wattributes diagnostics need to add -DPYBIND11_NOINLINE_FORCED -Wno-attributes to the g++ command lines. However, the measured shared-library size saving when using noinline are only 1.7% for CUDA, -0.2% for GCC7, and 0.0% for GCC8 (using -DCMAKE_BUILD_TYPE=MinSizeRel, the default under pybind11/tests).

@rwgk rwgk changed the title Removing last remnants of pragma block at the top of pybind11.h [Compromise] Removing last remnants of pragma block at the top of pybind11.h Aug 9, 2021
@rwgk rwgk changed the title [Compromise] Removing last remnants of pragma block at the top of pybind11.h Removing last remnants of pragma block at the top of pybind11.h Aug 12, 2021
@rwgk rwgk marked this pull request as ready for review August 12, 2021 23:18
// the default under pybind11/tests).
#if !defined(PYBIND11_NOINLINE_FORCED) && \
(defined(__CUDACC__) || (defined(__GNUC__) && (__GNUC__ == 7 || __GNUC__ == 8)))
# define PYBIND11_NOINLINE_DISABLED
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am guessing there is no clean way to detect if the warning is enabled is there? If there is seems like it might make sense to add it to the conditional.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am guessing there is no clean way to detect if the warning is enabled is there?

I don't think so, not even noinline is portable, even less so introspection of compiler-specific warnings related to it.

If there is seems like it might make sense to add it to the conditional.

If it was easy, yes.

But how likely is it that anyone cares enough about a 1.7% binary size reduction when building with CUDA? If someone does, they can add the suggested command line options. If they don't like that, they could send us a PR. — The group of affected people and the binary size saving both seem so marginal to me, I cannot imagine it's worth anyone's time, cost vs benefit.

@rwgk rwgk merged commit 1bcd94c into pybind:master Aug 14, 2021
@rwgk rwgk deleted the wattributes_compromise branch August 14, 2021 14:41
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label Aug 14, 2021
rwgk pushed a commit to rwgk/pybind11 that referenced this pull request Aug 30, 2021
…he comment and empty line that was added in PR pybind#3087; those were made obsolete by the pragma cleanup that concluded with PR pybind#3186.
@rwgk rwgk mentioned this pull request Aug 31, 2021
rwgk pushed a commit that referenced this pull request Aug 31, 2021
* Minor tweaks.

* Restoring tests/pybind11_tests.h version from master, removing just the comment and empty line that was added in PR #3087; those were made obsolete by the pragma cleanup that concluded with PR #3186.

* [ci skip] Restoring tests/test_enum.py from master.
rwgk pushed a commit that referenced this pull request Aug 31, 2021
…nt when underlying type is bool or of char type) (#3232)

* Minor tweaks.

* Restoring tests/pybind11_tests.h version from master, removing just the comment and empty line that was added in PR #3087; those were made obsolete by the pragma cleanup that concluded with PR #3186.

* More-to-the-point test for Python 3.
@henryiii henryiii removed the needs changelog Possibly needs a changelog entry label Sep 16, 2021
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