Adds -fopenmp flag to omp kernels + adds c++20 omp kernel #100
Conversation
mcbarton
force-pushed
the
Add-fopenmp-flag-for-omp-kernels
branch
from
f1f309b
to
d6a132b
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #100 +/- ##
==========================================
+ Coverage 53.66% 62.66% +9.00%
==========================================
Files 15 15
Lines 600 600
Branches 59 59
==========================================
+ Hits 322 376 +54
+ Misses 278 224 -54 |
mcbarton
force-pushed
the
Add-fopenmp-flag-for-omp-kernels
branch
from
5a9061e
to
658ebae
Compare
|
@anutosh491 Are you able to review this PR, and merge if your happy with it? |
|
Sure I can have a look in a while. |
|
Can we add a test for making sure that openmp works? IIRC, @alexander-penev and I had something as a demo in xeus-clang-repl somewhere... |
Yes, i'll add a test tomorrow and cc you when it passes. |
vgvassilev
requested review from
JohanMabille and
alexander-penev
and removed request for
JohanMabille
test/test_xcpp_kernel.py
Outdated
| assert reply is not None | ||
| self.assertEqual(reply["msg_type"], "is_complete_reply") | ||
| self.assertEqual(reply["content"]["status"], "complete") | ||
| self.assertEqual(reply['content']['matches'], '2') |
There was a problem hiding this comment.
Is that enough to make sure that we spawn multiple threads?
There was a problem hiding this comment.
omp_set_thread_num(4) in the code will make sure we use use 4 threads for all parallel regions.
There was a problem hiding this comment.
My issue at the moment is getting the test to pass. Trying to see whether the output comes out as 2 as expected, since only that thread should output.
There was a problem hiding this comment.
Can’t we call get_num_threads and make sure the result is different than 1?
There was a problem hiding this comment.
I'm open to suggestions on what code you'd like me to try. At the moment I can't get the code I am trying to run to work, to even be able to test it. It just timeouts whenever I try and run my code, with no useful output as to what caused the timeout.
There was a problem hiding this comment.
@alexander-penev, you are the one that made this work last time for xeus-clang-repl, any clues?
@mcbarton, can we maybe add a test in CppInterOp to make sure it works in openmp mode?
There was a problem hiding this comment.
@vgvassilev yes i'll look into an openmp test for CppInterOp
test/test_xcpp_kernel.py
Outdated
| code_omp='#include <iostream>\n#include "omp.h"\n#include "clang/Interpreter/CppInterOp.h"\nCpp::LoadLibrary("libomp");omp_set_thread_num(4);#pragma omp parallel\n{\nif(omp_get_thread_num()==2) std::cout<<omp_get_thread_num()<<std::endl;}' | ||
| def test_xcpp_omp(self): | ||
| self.flush_channels() | ||
| reply, output_msgs = self.execute_helper(code=self.code_omp,timeout=100) |
There was a problem hiding this comment.
Try to dump reply and output_msgs as string.
There was a problem hiding this comment.
I now have it running without timeout. It says it cannot find the omp header.
There was a problem hiding this comment.
@vgvassilev @alexander-penev
The error I got after I managed to avoid the timeout issue is
input_line_11:2:10: fatal error: 'omp' file not found
E - 2 | #include
E - | ^~~~~
E - Failed to parse via ::process:Parsing failed.
There was a problem hiding this comment.
We seem to have failed to pass the relevant -I or we did not install openmp?
There was a problem hiding this comment.
I've managed to make progress in that it now finds omp.h and runs things such as Cpp::LoadLibrary("libomp"),omp_get_max_threads() and omp_set_thread_num(). Now I just need to work out why it doesn't like my parallel section.
There was a problem hiding this comment.
The issue looks like its with the line #pragma omp parallel, but unsure what exactly it doesn't seem to like.
mcbarton
force-pushed
the
Add-fopenmp-flag-for-omp-kernels
branch
from
36978f0
to
328b5cb
Compare
mcbarton
force-pushed
the
Add-fopenmp-flag-for-omp-kernels
branch
from
d1ba6ad
to
25a8085
Compare
|
@alexander-penev how did you get the openmp library in xeus-clang-repl? That appears to be what is holding back this PR. I've tried multiple different ways without sucess, but I cannot get Cpp::LoadLibrary to load the libomp library, to be able to test the openmp kernel. |
I remember it was a huge mess before we sorted it out. I have no memory what we did though. However, it should be in the Dockerfile of xeus-clang-repl... |
This PR adds the openmp kernels (with the addition of a c++20 openmp kernel), setting the flag -fopenmp for those kernels which need it. @vgvassilev This PR is ready for review.