Support NVidia HPC SDK (PGI) by andriish · Pull Request #2475 · pybind/pybind11

andriish · 2020-09-09T16:41:34Z

In this PR:

Support NVidia HPC SDK with PG compilers
Factor out CMake filter function
Allow CMake to build even if pytest is missing (still shows warning)

Closes#2214 and closes#2407.

henryiii · 2020-09-09T16:48:13Z

+ if (WIN32)
+ target_compile_options(${target} PRIVATE -Wc,--pending_instantiations=0 )
+ else()
+ target_compile_options(${target} PRIVATE -Wc,--pending_instantiations=0 -D__GCC_ATOMIC_TEST_AND_SET_TRUEVAL=1)


Are these always required, or just for the tests? If always, maybe they should go into pybind11Common for pybind11 or some similar target.

-Wc,--pending_instantiations=0 is about the recursion level in templates. If I recall that correctly.
So, well, in some codes it will not be needed. But in the others the compilation will fail without this option.
Note that this option makes compilation significantly slower.
-D__GCC_ATOMIC_TEST_AND_SET_TRUEVAL=1 was deduced from experiments with old compilers on Linux.

andriish · 2020-09-09T16:53:15Z

Hi @henryiii ,

just to avoid misunderstanding, I will quote myself from the #2407
"I'm not sure if I will have any time to actually implement the full support, so the pull request is marked as work in progress.
Feel free to close it at any time.".
I mean that some things can be picked up from this merge request and might be useful. On some I can comment, the others would require some really dedicated work and changes in the build system. Not something that I have time for (even if I would like to in some cases). So most things are up you and other developers.

Best regards,
Andrii

henryiii · 2020-09-09T20:10:02Z

+ - 7 # GCC 4.8
+ - 8


Any reason you are building twice, with identical compilers, just with a different base system? You aren't using the host GCC.

Hi @henryiii ,
There are differences, e.g. Python and cmake. But in general, I would consider CI to be an "environment test" rather than a test of some specific compiler version. The later has little practical value, as for me.
Best regards,
Andrii

henryiii · 2020-09-09T23:41:01Z

Have you tried running the tests? It seems like many of them error out, mostly with things like "SystemError: Exception escaped from default exception translator!". Looks like this is pretty broken without this.

henryiii · 2020-09-10T04:34:14Z

Looked into PGI a bit more and realized we are probably making a fundamental mistake. Will fix tomorrow.

andriish · 2020-09-10T07:55:23Z

Have you tried running the tests

Not the tests with exceptions. I see no hope for simple fixes for them.

we are probably making a fundamental mistake

I'm intrigued. Do you mean that it is not possible to support their implementation of stl? Or the approach is wrong?
Ok. Will just wait.

henryiii · 2020-09-10T12:42:01Z

I looked into PGI and it has complete C++11 support since 2015. So I then added #include <exception> (we do not run include-what-you-use, obviously) and that fixed all the issues with "missing" std::exception and such; PGI's stdlib is less "leaky" and we didn't get <exception> for free.

I re-enabled all the tests, and now only one test fails:

> assert msg(capture) == strip_comments("""
noisy new # allocation required to attempt first overload
noisy delete # have to dealloc before considering factory init overload
noisy new # pointer factory calling "new", part 1: allocation
NoisyAlloc(double 1.5) # ... part two, invoking constructor
---
~NoisyAlloc() # Destructor
noisy delete # operator delete
""")
E assert --- actual / +++ expected
E noisy new
E + noisy delete
E noisy new
E NoisyAlloc(double 1.5)
E ---
E ~NoisyAlloc()
E noisy delete
../../tests/test_factory_constructors.py:358: AssertionError

henryiii · 2020-09-10T12:46:53Z

There are still quite a few warnings - we manually silence quite a few warnings for GCC/Clang, which obviously doesn't scale very well. I'm not sure how to manually silence them for PGI in-source.

andriish · 2020-09-10T14:19:01Z

are still quite a few warnings

 statement is unreachable
return false;

Yes. That is why"more different compilers" is good for debug.

how to manually silence them for PGI in-source.

https://forums.developer.nvidia.com/t/disable-unreachable-warnings/133575

Unfortunately that is "all or nothing".

Best regards,

Andrii

Added new CI config Added new trigger Changed CI workflow name Debug CI Debug CI Debug CI Debug CI Added flags fro PGI Disable Eigen Removed tests that fail Uncomment lines

fix: minor style cleanup tests: support skipping ci: remove and tighten a bit fix: try msvc workaround for pgic

henryiii · 2020-09-11T22:01:34Z

I left the verbose in, wasn't too long. I've reordered that test and now pytest is no longer a FATAL_ERROR, so you can build without it.

andriish · 2020-09-11T22:01:51Z

Can you check the file exception locally?

been there.
Literally the code from https://en.cppreference.com/w/cpp/error/exception_ptr

nvc++ 1.cc "1.cc", line 6: error: namespace "std" has no member "exception_ptr"
void handle_eptr(std::exception_ptr eptr) // passing by value is ok

Also, grepping the nvidia installation shows that "exception_ptr" is located only in the libstdc++.ipl

henryiii · 2020-09-11T22:05:59Z

I think it's getting it from libstdc++ on your system (GCC 4.8), not from the install.

andriish · 2020-09-11T22:08:50Z

I mean the definition. There is just one header with exception_ptr definition and that definition is behind very funny condition expressions.

andriish · 2020-09-11T22:17:49Z

Could you, please try this code locally on CentOS7 with and w/o defines?

#if defined(__PGIC__) and defined(__GXX_EXPERIMENTAL_CXX0X__) and (__cplusplus >= 201103L)
#ifndef __GCC_ATOMIC_TEST_AND_SET_TRUEVAL
#define __GCC_ATOMIC_TEST_AND_SET_TRUEVAL 1
#endif
#if !defined(ATOMIC_INT_LOCK_FREE)
#define ATOMIC_INT_LOCK_FREE 3
#include <bits/exception_ptr.h>
#undef ATOMIC_INT_LOCK_FREE
#endif
#endif
#include <iostream>
#include <string>
#include <exception>
#include <stdexcept>
void handle_eptr(std::exception_ptr eptr) // passing by value is ok
{
try {
if (eptr) {
std::rethrow_exception(eptr);
}
} catch(const std::exception& e) {
std::cout << "Caught exception \"" << e.what() << "\"\n";
}
}
int main()
{
std::exception_ptr eptr;
try {
std::string().at(1); // this generates an std::out_of_range
} catch(...) {
eptr = std::current_exception(); // capture
}
handle_eptr(eptr);
} // destructor for std::out_of_range called here, when the eptr is destructed

andriish · 2020-09-11T22:18:15Z

with -std=c++11

henryiii · 2020-09-11T22:38:52Z

I was able to reproduce locally (in docker). Very weird, no idea why this is different compared to docker on GHA! Try this:

cmake3 -S pybind11 -B build -DDOWNLOAD_CATCH=ON \
-DCMAKE_CXX_STANDARD=11 \
-DPYTHON_EXECUTABLE=$(python3 -c "import sys; print(sys.executable)") \
-DCMAKE_CXX_FLAGS="-Wc,--pending_instantiations=0 -D__GCC_ATOMIC_TEST_AND_SET_TRUEVAL=1 -D__GCC_ATOMIC_INT_LOCK_FREE=3" \
-DPYBIND11_TEST_FILTER="test_smart_ptr.cpp;test_virtual_functions.cpp"

henryiii · 2020-09-11T22:40:04Z

You are running natively, right (not containerized)?

andriish · 2020-09-11T22:51:00Z

are running natively, right

Yes. Always! (no, sometimes in VB).

Ok, now more instanity. PGI 19.10 works perfectly.

henryiii · 2020-09-11T22:51:04Z

That compiles for me. Is "3" even a valid number for that? https://en.cppreference.com/w/c/atomic/ATOMIC_LOCK_FREE_consts

andriish · 2020-09-11T22:52:07Z

Yes. Basically the <bits/exception_ptr.h> has condition bla-bla>2.

henryiii · 2020-09-11T22:53:42Z

I think PGI 20.7 doesn't support GCC 4.8 (even though it probably should if they are providing a repo for it, I bet it can be filed as a bug). The compiler is supposed to define that, and I think that's been dropped in 20.7. Except when running in GHA, in which case it magically doesn't drop that. That's about where I am. :S

henryiii · 2020-09-11T22:54:25Z

In short, since there is a command line workaround, I don't think we should hack the source. Probably report it as a bug with 20.7 and CentOS 7 and move on.

henryiii · 2020-09-11T22:56:22Z

We should probably link to this PR in somewhere, so if it breaks we can add the workaround to the CI.

andriish · 2020-09-11T22:58:25Z

magically

Container magic as is.

Ok. Agree to your suggestions. Basically to refine them:

put these flags into the cmake file with condition-specific to this compiler major and minor version
fill a bug report to Nvidia. I will do this and put you to cc.
Merge if you are ok with the changes. That is totally up to you.

What is your opinion?

Best regards,

Andrii

henryiii · 2020-09-11T23:01:35Z

I would not add the changes to CMake. Maybe put a note in. But this is a user workaround that has nothing to do with pybind11 (it's needed to compile the example code from cppref above), and it's OS (probably stdlib) + compiler specific. Just a note in the changlog, perhaps, saying this is supported but on old systems, you may need the workaround in issue #2475.

henryiii · 2020-09-11T23:02:04Z

(I'll try this locally on CentOS 8 later tonight)

andriish · 2020-09-11T23:04:59Z

But this is a user workaround

Yes, so that should not appear in the code, but it is ok to put it in the test suite.

I also think this is a pure bug, not an "unsupported version".

henryiii · 2020-09-11T23:08:39Z

We don't understand the details of the bug, what versions it affects, or why the workaround is needed/what it affects. Hardcoding it into the test suite hides that the workaround is needed. Let's leave it as a note. (Unless another maintainer feels otherwise).

andriish · 2020-09-11T23:21:46Z

Ok. Note is ok. One ca also issue a message during cmake configuration in case pgi is detected.

henryiii · 2020-09-11T23:53:53Z

message during cmake configuration in case pgi is detected

Once we understand it better, that's a possibility.

henryiii · 2020-09-12T00:18:59Z

When I did this by hand, I got, in CentOS 8:

"/usr/include/c++/8/bits/atomic_base.h", line 210: error: identifier
"__GCC_ATOMIC_TEST_AND_SET_TRUEVAL" is undefined
{ return __i ? __GCC_ATOMIC_TEST_AND_SET_TRUEVAL : 0; }

(Again, this is a problem out of the box, not with pybind11).

Looking here: https://forums.developer.nvidia.com/t/support-for-atomic-in-libstdc-missing/135403/4

It looks a bit to me like that it was rather designed for an HPC admin to create a siterc that has defines like this that describe the HPC hardware that you will be targeting (since you compile on a host node, not a compute node, in general, and they may not be homogenous). That also is likely why it uses environment-modules; this is really not intended for "normal" users to drop in and go. The docker container is set up properly, but the rest is "standard PGI" configuration that you have to do.

(I could totally be wrong here. I also rather don't think they are targeting normal users yet, just HPC clusters)

henryiii · 2020-09-12T02:15:49Z

Let me know / CC me on the issue with NVIDIA! For now, this should now be better / compilable via flags or site configuration instead of source changes.

henryiii · 2020-09-12T03:24:15Z

The rpm repo seems to be a little unstable. If it causes build to fail occasionally in the long run, I'll move to the container for the main test suite and make the two CentOS builds weekly or something like that.

YannickJadoul · 2020-09-12T20:58:23Z

Congrats on getting this to work, @henryiii & @andriish! :-)