Resolve libomp test errors#27073
Conversation
|
Thanks a lot for looking into this. I'm looking forward to this for supporting openmp with clang-repl in the browser ! |
|
@anutosh491 tests are resolved. But I haven't tried it on my app yet. |
|
hi, @JAicewizard have you taken a look yet? |
duerrbaby
changed the title
|
@sbc100 can you help to review? |
|
@JAicewizard @sbc100 Hi again, does anyone have the time to review and hopefully merge this? |
sbc100
left a comment
sbc100
left a comment
There was a problem hiding this comment.
Can we call the new directory system/lib/openmp to maybe the llvm directory name?
Can you update the PR title to better reflect what this is? Something like "Add openmp library from llvm" maybe?
| never_force = True | ||
| cflags = [ | ||
| '-O3', '-DNDEBUG', '-pthread', | ||
| '-D_GNU_SOURCE', '-U_GLIBCXX_ASSERTIONS', '-D_GLIBCXX_NO_ASSERTIONS', |
There was a problem hiding this comment.
Why are there GLIBCXX macros here? We don't glibc anywhere so I'm surprised to see these.
| cflags = [ | ||
| '-O3', '-DNDEBUG', '-pthread', | ||
| '-D_GNU_SOURCE', '-U_GLIBCXX_ASSERTIONS', '-D_GLIBCXX_NO_ASSERTIONS', | ||
| '-fno-exceptions', '-fno-rtti', |
There was a problem hiding this comment.
I believe both of these are the default so should not be needed.
| @@ -6245,6 +6245,7 @@ def test_modularize_instantiation_error(self): | |||
| # The flakiness of this test is very high on macOS so just disable it | |||
| # completely. | |||
| @no_mac('https://github.com/emscripten-core/emscripten/issues/19683') | |||
| @no_deno('Also flaky on Deno') | |||
There was a problem hiding this comment.
Please revert this line. We could consider adding this, but it should be in a different PR
| } | ||
| """ | ||
| self.cflags += ["-fopenmp=libomp"] | ||
| self.do_run(src, "") |
There was a problem hiding this comment.
Can you include the cflags in the do_run line. e.g. self.do_run(src, "", cflags=[...]).
| src = r""" | ||
| #include <omp.h> | ||
| int main(void) { | ||
| omp_get_max_threads(); |
There was a problem hiding this comment.
What do we expect this to return?
Should we at least assert that its greater than zero?
| os.mkdir("build") | ||
| os.chdir("build") | ||
|
|
||
| subprocess.call( |
There was a problem hiding this comment.
Can you use subprocess.run here and below instead of call.
| ) | ||
| subprocess.call("ninja") | ||
| subprocess.call(["env", "DESTDIR=.", "ninja", "install"]) | ||
| os.chdir(cwd) |
There was a problem hiding this comment.
Instead of calling os.chdir you can just pass cwd=build_dir to the tree subproceses
| ] | ||
| ) | ||
| subprocess.call("ninja") | ||
| subprocess.call(["env", "DESTDIR=.", "ninja", "install"]) |
There was a problem hiding this comment.
Maybe just pass --destdir=. to cmake above instead of using env here?\
Also, maybe we should not use the build dir as the install dir? That could be confusing.
| shutil.copy2( | ||
| os.path.join(upstream_build_src, "kmp_i18n_default.inc"), | ||
| local_src, | ||
| ) |
There was a problem hiding this comment.
Maybe use a loop over a file list here?
| @@ -0,0 +1,126 @@ | |||
| #!/usr/bin/env python3 | |||
| # Copyright 2023 The Emscripten Authors. All rights reserved. | |||
4 participants
Original PR here: #25937