Using Werror-all for Intel #2948

philbucher · 2021-04-13T14:17:10Z

Description

For turning compiler warnings into errors with Intel I think it is necessary to use Werror-all instead of Werror
Source

EDIT Werror exists too, but Werror-all is more strict

I think there will be some minor things to be fixed, at least I had to do this in my project.
Lets see if the CI picks it up :)

Suggested changelog entry:

Changed ``Werror`` to stricter ``Werror-all`` for Intel compiler and fixed minor issues

philbucher · 2021-04-13T14:23:26Z

ok the CI found the same issues I had in my code:

In file included from /home/runner/work/pybind11/pybind11/tests/pybind11_tests.h(2),
                 from /home/runner/work/pybind11/pybind11/tests/pybind11_cross_module_tests.cpp(10):
/home/runner/work/pybind11/pybind11/include/pybind11/pybind11.h(147): error #2282: unrecognized GCC pragma
  #  pragma GCC diagnostic push
                           ^

In file included from /home/runner/work/pybind11/pybind11/tests/pybind11_tests.h(2),
                 from /home/runner/work/pybind11/pybind11/tests/pybind11_tests.cpp(10):
/home/runner/work/pybind11/pybind11/include/pybind11/pybind11.h(147): error #2282: unrecognized GCC pragma
  #  pragma GCC diagnostic push
                           ^

In file included from /home/runner/work/pybind11/pybind11/tests/pybind11_tests.h(2),
                 from /home/runner/work/pybind11/pybind11/tests/pybind11_cross_module_tests.cpp(10):
/home/runner/work/pybind11/pybind11/include/pybind11/pybind11.h(148): error #2282: unrecognized GCC pragma
  #  pragma GCC diagnostic ignored "-Wplacement-new"
                           ^

In file included from /home/runner/work/pybind11/pybind11/tests/pybind11_tests.h(2),
                 from /home/runner/work/pybind11/pybind11/tests/pybind11_cross_module_tests.cpp(10):
/home/runner/work/pybind11/pybind11/include/pybind11/pybind11.h(152): error #2282: unrecognized GCC pragma
  #  pragma GCC diagnostic pop
                           ^

In file included from /home/runner/work/pybind11/pybind11/tests/pybind11_tests.h(2),
                 from /home/runner/work/pybind11/pybind11/tests/pybind11_tests.cpp(10):
/home/runner/work/pybind11/pybind11/include/pybind11/pybind11.h(148): error #2282: unrecognized GCC pragma
  #  pragma GCC diagnostic ignored "-Wplacement-new"
                           ^

In file included from /home/runner/work/pybind11/pybind11/tests/pybind11_tests.h(2),
                 from /home/runner/work/pybind11/pybind11/tests/pybind11_tests.cpp(10):
/home/runner/work/pybind11/pybind11/include/pybind11/pybind11.h(152): error #2282: unrecognized GCC pragma
  #  pragma GCC diagnostic pop
                           ^

In file included from /home/runner/work/pybind11/pybind11/tests/pybind11_tests.h(2),
                 from /home/runner/work/pybind11/pybind11/tests/pybind11_cross_module_tests.cpp(10):
/home/runner/work/pybind11/pybind11/include/pybind11/pybind11.h(2287): error #2282: unrecognized GCC pragma
  #  pragma GCC diagnostic pop
                           ^

In file included from /home/runner/work/pybind11/pybind11/tests/pybind11_tests.h(2),
                 from /home/runner/work/pybind11/pybind11/tests/pybind11_tests.cpp(10):
/home/runner/work/pybind11/pybind11/include/pybind11/pybind11.h(2287): error #2282: unrecognized GCC pragma
  #  pragma GCC diagnostic pop
                           ^

my second commit should fix them

henryiii · 2021-04-13T14:37:35Z

tests/CMakeLists.txt

      target_compile_options(${target_name} PRIVATE -Werror)
+    elseif(CMAKE_CXX_COMPILER_ID MATCHES "Intel")


Is this true for both Intel compilers? (IntelLLVM and classic Intel; both will match here)

thx for the fast reply

No clue tbh. This is the first time I learned about IntelLLVM. I took a brief look at their docs but couldn't find it

Are you sure it would match both? According to CMake the CMAKE_CXX_COMPILER_ID for IntelLLVM is IntelLLVM

also checking the docs more in detail I noticed that Werror does exist, but it is not as strict as Werror-all. Maybe Werror-all is too strict? What do you think

MATCH will find the word anywhere; Clang and AppleClang MATCH. So IntelLLVM will MATCH Intel. (I guess they chose LLVM to avoid MATCHing with Clang, as IntelClang would have matched, and due to the AppleClang compiler, that's a very common expression in CMakeLists.

The Clang-based Intel compiler is the "new" compiler, and the one they are supporting going forward. The old compiler is now renamed "Classic Intel" (as you can see in the docs), and is being killed off.

The new compiler is called "Intel® oneAPI DPC++/C++ Compiler", the old one "Intel® C++ Compiler Classic", as you can see here: https://software.intel.com/content/www/us/en/develop/tools/oneapi/all-toolkits.html

thx for the explanation.
I used STREQUAL now, please let me know if it is ok

henryiii · 2021-04-13T14:43:16Z

Ignore the unrelated failure for Python 3.10, that's due to pytest-dev/pytest#8539

philbucher · 2021-04-13T15:06:41Z

so now the ward issues are fixed, but there are two more warnings:

error #11074: Inlining inhibited by limit max-size 
error #11076: To get full report use -qopt-report=4 -qopt-report-phase ipo

I am not sure what to do about those. Maybe there are some limits or inlining specified? => see here

henryiii · 2021-04-13T16:39:25Z

I think we set something similar for the NVHPC (PGI) compiler, which I think shares some history with Intel (IIRC). -no-inline-max-size might force it? Though from what I can tell, it uses default heuristics, and that probably causes a few requested inlines to not happen (and hence, the warning becoming an error above). Maybe we can find the size with -qopt-report=4 -qopt-report-phase ipo and then set it a little larger than that with -max-size?

rwgk · 2021-04-14T00:04:41Z

EDIT 4/14: I was wrong before (below), I overlooked that this PR does not affect the pragma block.

Note to myself: the smart_holder branch will need to be updated accordingly.

For the smart_holder branch I had to move the pragma block in pybind11.h (#2873), which wasn't supported by anyone unfortunately.

philbucher · 2021-04-14T07:44:08Z

I think we set something similar for the NVHPC (PGI) compiler, which I think shares some history with Intel (IIRC). -no-inline-max-size might force it? Though from what I can tell, it uses default heuristics, and that probably causes a few requested inlines to not happen (and hence, the warning becoming an error above). Maybe we can find the size with -qopt-report=4 -qopt-report-phase ipo and then set it a little larger than that with -max-size?

this is what I get when enabling the report:

INLINING OPTION VALUES:
  -inline-factor: 100
  -inline-min-size: 30
  -inline-max-size: 230
  -inline-max-total-size: 2000
  -inline-max-per-routine: disabled
  -inline-max-per-compile: disabled

I then thought it would probably be better to disable the warnings for the following reasons:

The log is very large (~500K lines). Parsing it to find the max numbers is possible but I think it is dangerous if in the future the size increases further then suddenly the intel build will fail for a seemingly unrelated change
I also thought of removing the limits completely but this is
- a behavior change
- can lead to large memory consumption (which I already fought with quite a lot with GitHub Actions)
the limits for inlining are already used, but they werent noticed as the compiler didn't issue the warning

Bottom line now I disabled them, which I think is the best solution

Thx for your help, please let me know what you think

philbucher · 2021-04-14T08:32:55Z

cool now the CI passes (except the build you said doesn't work)

rwgk

Looks good to me.

include/pybind11/pybind11.h

tests/CMakeLists.txt

Co-authored-by: Henry Schreiner <[email protected]>

philbucher · 2021-04-14T16:51:19Z

thanks for the fast responses and help!

henryiii · 2021-04-14T18:01:33Z

Thanks!

correcting Werror for Intel

7694568

philbucher requested a review from henryiii as a code owner April 13, 2021 14:17

adding ward for Intel

4bf349b

philbucher added 2 commits April 13, 2021 16:27

adding wards for intel

9b0d184

another ward for Intel

440e651

henryiii reviewed Apr 13, 2021

View reviewed changes

missed one intel ward

70ff84c

philbucher changed the title ~~correcting Werror for Intel~~ Using Werror-all for Intel Apr 13, 2021

rwgk self-requested a review April 14, 2021 00:00

philbucher added 3 commits April 14, 2021 09:09

exact match for intel compiler

f8eaec6

removing inline limits

baf6f41

disable warnings about inline limits

fedf30b

philbucher added 3 commits April 14, 2021 09:45

formatter suggestion

9e7b923

more indent

69c5601

hopefully make formatter happy

4f02355

rwgk approved these changes Apr 14, 2021

View reviewed changes

include/pybind11/pybind11.h Outdated Show resolved Hide resolved

addressed review

7b596ea

henryiii reviewed Apr 14, 2021

View reviewed changes

tests/CMakeLists.txt Outdated Show resolved Hide resolved

philbucher and others added 2 commits April 14, 2021 16:37

fix &&

fbed50a

Update tests/CMakeLists.txt

9ec6559

Co-authored-by: Henry Schreiner <[email protected]>

henryiii approved these changes Apr 14, 2021

View reviewed changes

henryiii merged commit 62976cf into pybind:master Apr 14, 2021

henryiii added bug compiler issue compiler: intel Related to the Intel compilers labels Apr 14, 2021

github-actions bot added the needs changelog Possibly needs a changelog entry label Apr 14, 2021

philbucher deleted the patch-1 branch April 14, 2021 18:23

henryiii removed the needs changelog Possibly needs a changelog entry label Jul 13, 2021

rwgk mentioned this pull request Feb 10, 2023

FWD pybind11 google/pybind11clif#2948

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Using Werror-all for Intel #2948

Using Werror-all for Intel #2948

philbucher commented Apr 13, 2021 •

edited by henryiii

Loading

philbucher commented Apr 13, 2021

henryiii Apr 13, 2021

philbucher Apr 13, 2021

henryiii Apr 13, 2021

henryiii Apr 13, 2021

philbucher Apr 14, 2021

henryiii commented Apr 13, 2021

philbucher commented Apr 13, 2021

henryiii commented Apr 13, 2021

rwgk commented Apr 14, 2021 •

edited

Loading

philbucher commented Apr 14, 2021

philbucher commented Apr 14, 2021

rwgk left a comment

philbucher commented Apr 14, 2021

henryiii commented Apr 14, 2021

		target_compile_options(${target_name} PRIVATE -Werror)
		elseif(CMAKE_CXX_COMPILER_ID MATCHES "Intel")

Using Werror-all for Intel #2948

Using Werror-all for Intel #2948

Conversation

philbucher commented Apr 13, 2021 • edited by henryiii Loading

Description

Suggested changelog entry:

philbucher commented Apr 13, 2021

henryiii Apr 13, 2021

Choose a reason for hiding this comment

philbucher Apr 13, 2021

Choose a reason for hiding this comment

henryiii Apr 13, 2021

Choose a reason for hiding this comment

henryiii Apr 13, 2021

Choose a reason for hiding this comment

philbucher Apr 14, 2021

Choose a reason for hiding this comment

henryiii commented Apr 13, 2021

philbucher commented Apr 13, 2021

henryiii commented Apr 13, 2021

rwgk commented Apr 14, 2021 • edited Loading

philbucher commented Apr 14, 2021

philbucher commented Apr 14, 2021

rwgk left a comment

Choose a reason for hiding this comment

philbucher commented Apr 14, 2021

henryiii commented Apr 14, 2021

philbucher commented Apr 13, 2021 •

edited by henryiii

Loading

rwgk commented Apr 14, 2021 •

edited

Loading