This is an archive of the discontinued LLVM Phabricator instance.

Implementation of OMPT as specified in OpenMP 5.0 Preview 1
ClosedPublic

Authored by protze.joachim on Sep 22 2017, 12:36 PM.

Details

Summary

This patch updates the implementation of OMPT to the specification in the upcoming OpenMP 5.0 version.

Half of the touched code adds test cases to test the various OMPT event callbacks. The code is tested to work with latest clang, GNU and Intel compiler. The implementation is optimized for low overhead when no tool is attached shifting the cost to execution with tool attached.

The code is developed in:

https://github.com/OpenMPToolsInterface/LLVM-openmp

The diff can also be viewed at:

https://github.com/OpenMPToolsInterface/LLVM-openmp/pull/2/files

Diff Detail

Repository
rL LLVM

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
omalyshe added inline comments.Oct 3 2017, 6:29 AM
runtime/src/kmp_dispatch.cpp
1235 ↗(On Diff #116382)

The comment is not relevant any more

2544 ↗(On Diff #116382)

Should it be #if OMPT_SUPPORT && OMPT_OPTIONAL in all occurrences below?

runtime/src/kmp_global.cpp
309 ↗(On Diff #116382)

LIBOMP_OMPT_SUPPORT -> OMPT_SUPPORT

runtime/src/kmp_gsupport.cpp
36 ↗(On Diff #116382)

Should be under OMPT_OPTIONAL?

45 ↗(On Diff #116382)

Should be under OMPT_OPTIONAL?

67 ↗(On Diff #116382)

Should be under OMPT_OPTIONAL?

77 ↗(On Diff #116382)

Should be under OMPT_OPTIONAL?

184 ↗(On Diff #116382)

Should be under OMPT_OPTIONAL?

197 ↗(On Diff #116382)

Should be under OMPT_OPTIONAL?

220 ↗(On Diff #116382)

Should be under OMPT_OPTIONAL and more others below?

256 ↗(On Diff #116382)

Should be under OMPT_OPTIONAL?

645 ↗(On Diff #116382)

Should be under OMPT_OPTIONAL?

833 ↗(On Diff #116382)

Should be under OMPT_OPTIONAL?

omalyshe added inline comments.Oct 3 2017, 8:43 AM
runtime/src/kmp_sched.cpp
142 ↗(On Diff #116382)

Remove the comment

198 ↗(On Diff #116382)

Remove the comment

231 ↗(On Diff #116382)

Remove the comment

392 ↗(On Diff #116382)

Remove the comment

runtime/src/kmp_settings.cpp
4360 ↗(On Diff #116382)

LIBOMP_OMPT_SUPPORT -> OMPT_SUPPORT

4627 ↗(On Diff #116382)

LIBOMP_OMPT_SUPPORT -> OMPT_SUPPORT

runtime/src/kmp_tasking.cpp
602 ↗(On Diff #116382)

Remove extra empty line

894 ↗(On Diff #116382)

to be removed

900 ↗(On Diff #116382)

to be removed

1593 ↗(On Diff #116382)

Should it be under OMPT_OPTIONAL?

hbae edited edge metadata.Oct 3 2017, 1:32 PM

Just a few small things from me.

runtime/src/exports_so.txt
29 ↗(On Diff #116382)

This is not necessary since we have omp_control_tool now.

runtime/src/include/50/ompt.h.var
691 ↗(On Diff #116382)

Do we still need this function?

runtime/src/kmp_csupport.cpp
2994 ↗(On Diff #116382)

Yes, it should be __ompt_get_mutex_impl_type(user_lock)

runtime/src/kmp_tasking.cpp
458 ↗(On Diff #116382)

ompt_tool -> ompt_start_tool

462 ↗(On Diff #116382)

Remove the commented if condition.

555 ↗(On Diff #116382)

task_data is never used.

omalyshe added inline comments.Oct 4 2017, 7:14 AM
runtime/src/kmp_barrier.cpp
1892 ↗(On Diff #116382)

OMPT_CUR_TASK_DATA(this_thr) is defined for this.

runtime/src/kmp_csupport.cpp
489 ↗(On Diff #116382)

OMPT_CUR_TASK_INFO () to be used

494 ↗(On Diff #116382)

OMPT_CUR_TASK_DATA(this_thr) is defined for this

runtime/src/kmp_ftn_entry.h
361 ↗(On Diff #116382)

OMPT_CUR_TASK_INFO() to be used

runtime/src/kmp_runtime.cpp
1209 ↗(On Diff #116382)

OMPT_CUR_TASK_INFO() to be used

1402 ↗(On Diff #116382)

OMPT_CUR_TASK_DATA(this_thr) is defined for this

7225 ↗(On Diff #116382)

OMPT_CUR_TASK_DATA(this_thr) is defined for this

7226 ↗(On Diff #116382)

OMPT_CUR_TEAM_DATA to be used

7233 ↗(On Diff #116382)

OMPT_CUR_TEAM_INFO() to be used

runtime/src/kmp_tasking.cpp
1612 ↗(On Diff #116382)

OMPT_CUR_TEAM_DATA() to be used

runtime/src/kmp_wait_release.h
111 ↗(On Diff #116382)

What is pteam?

225 ↗(On Diff #116382)

OMPT_CUR_TEAM_DATA(this_thr) to be used

226 ↗(On Diff #116382)

OMPT_CUR_TASK_DATA(this_thr) to be used

runtime/src/ompt-specific.cpp
295 ↗(On Diff #116382)

OMPT_CUR_TEAM_INFO(thr) to be used

296 ↗(On Diff #116382)

OMPT_CUR_TEAM_INFO(thr) to be used

300 ↗(On Diff #116382)

OMPT_CUR_TASK_INFO() to be used
(and in the line above)

310 ↗(On Diff #116382)

OMPT_CUR_TEAM_INFO(thr) to be used

311 ↗(On Diff #116382)

OMPT_CUR_TASK_INFO() to be used

322 ↗(On Diff #116382)

OMPT_CUR_TEAM_INFO(thr) to be used

327 ↗(On Diff #116382)

OMPT_CUR_TASK_INFO() to be used

runtime/src/ompt-specific.h
57 ↗(On Diff #116382)

never used

59 ↗(On Diff #116382)

never used

61 ↗(On Diff #116382)

never used

63 ↗(On Diff #116382)

never used

Changes to clang-format factored out into D38837

This diff fixes many of the comments. (I will walk over the comments and mark them as done)
This diff is against master + parent dependent differentals

protze.joachim marked 59 inline comments as done.Oct 14 2017, 9:06 AM

Marked comments that are solved by latest diff as done.

runtime/src/include/50/ompt.h.var
386 ↗(On Diff #116382)

This name is given by the spec. If you really perfer this modification, please create an issue in the OMPT spec repository

512 ↗(On Diff #116382)

I agree, this would be better. But we need to change the spec first, before we update this in the implementation.

runtime/test/lit.cfg
126–128 ↗(On Diff #116382)

The idea of CHECK-WAIT is to allow testing of a runtime, that does not support "optional" events.
If OMPT_OPTIONAL is off, only CHECK, but non of the optional CHECK-X should be checked

protze.joachim marked 12 inline comments as done.

Implemented more requested changes

protze.joachim marked 8 inline comments as done.Oct 14 2017, 11:07 AM

Applied old clang-format file.

A few more comments

runtime/src/kmp_ftn_entry.h
347–349 ↗(On Diff #119252)

Remove the xexpand macro here since we aren't matching a version symbol with libgomp

1287 ↗(On Diff #119252)

Remove this as well for the same reason

1359 ↗(On Diff #119252)

And remove this too.

Addressed latest comments by jlpeyton

runtime/src/kmp_ftn_os.h
54 ↗(On Diff #119359)

This is inconsistent with the ifs below: either OMPT_SUPPORT or OMP_50_ENABLED

I would say OMP_50_ENABLED for the user facing functions. The implementation is a stub if OMPT_SUPPORT is off.

jlpeyton added inline comments.Oct 17 2017, 1:13 PM
runtime/src/kmp_ftn_os.h
54 ↗(On Diff #119359)

Yes, can you move this down below OMP_45_ENABLED and wrap it with the OMP_50_ENABLED guard.

419–421 ↗(On Diff #119359)

Same with this one.

Some more comments, there is still a lot of commented code that should be removed.
Another, higher level question: When should frame addresses be set? Currently, that's inconsistently guarded with OMPT_OPTIONAL. However, if I remember correctly this is mandatory?

runtime/CMakeLists.txt
324 ↗(On Diff #119359)

Nit: This commit id is probably in the fork.

runtime/src/include/50/ompt.h.var
224 ↗(On Diff #119359)

This file might not have been formatted? There are 4 spaces instead of 2 further down...

runtime/src/kmp_gsupport.cpp
155–156 ↗(On Diff #119359)

Remove

940 ↗(On Diff #119359)

Nothing to do with this patch but an interesting question...

1206–1211 ↗(On Diff #119359)

There is IF_OMPT_SUPPORT above, please collapse

runtime/src/kmp_runtime.cpp
1100–1104 ↗(On Diff #119359)

This formatting looks weird?

1208–1212 ↗(On Diff #119359)

Remove commented coe

1215–1216 ↗(On Diff #119359)

Remove

1223–1224 ↗(On Diff #119359)

Remove

1483 ↗(On Diff #119359)

Remove

1515–1516 ↗(On Diff #119359)

Remove

5946–5947 ↗(On Diff #119359)

Formatting looks weird

runtime/src/kmp_tasking.cpp
492–494 ↗(On Diff #119359)

__ompt_task_finish?

Hahnfeld added inline comments.Oct 17 2017, 5:10 PM
runtime/src/kmp_tasking.cpp
496–497 ↗(On Diff #119359)

Remove this code if that's guaranteed

1469–1470 ↗(On Diff #119359)

Remove

1700–1705 ↗(On Diff #119359)

Puh, that's ugly. Is this needed for performance reasons?

runtime/src/ompt-event-specific.h
25 ↗(On Diff #119359)

Remove

runtime/test/ompt/callback.h
70–92 ↗(On Diff #119359)

Remove

115–130 ↗(On Diff #119359)

Remove

runtime/test/ompt/cancel/cancel_parallel.c
15–18 ↗(On Diff #119359)

There is a macro for this!

runtime/test/ompt/loadtool/tool.c
1 ↗(On Diff #119359)

That's not really a test...

runtime/test/ompt/synchronization/taskwait.c
34–37 ↗(On Diff #119359)

Why are these deactivated?

runtime/test/ompt/tasks/task_types.c
111–112 ↗(On Diff #119359)

Deactivated?

omalyshe added inline comments.Oct 18 2017, 1:44 AM
runtime/src/kmp_gsupport.cpp
940 ↗(On Diff #119359)

It looks like this can be safely replaced with __kmp_omp_task. The eneral functionality is the same. The only loss would be some statistics (KMP_SET_THREAD_STATE_BLOCK(EXPLICIT_TASK)) that is not default anyway.

protze.joachim added inline comments.Oct 18 2017, 6:47 AM
runtime/src/include/50/ompt.h.var
224 ↗(On Diff #119359)

This file was not formatted.
clang-format has problems with above FOREACH macros and tries to indent each item by another level.

protze.joachim added inline comments.Oct 18 2017, 7:15 AM
runtime/test/ompt/loadtool/tool.c
1 ↗(On Diff #119359)

We need the source for the library to be loaded by the other test in this directory (tool_available.c). What is your suggestion for having the code and library available?

We could merge the two files and separate the code by two ifdefs. The test would then compile the same file with two different defines.

Hahnfeld added inline comments.Oct 18 2017, 7:19 AM
runtime/test/ompt/loadtool/tool.c
1 ↗(On Diff #119359)

Ah, now I understand the intention. Right, that would be one solution and would keep the code together...
Do we always need a RUN line? What happens if you just remove it?

protze.joachim added inline comments.Oct 18 2017, 7:31 AM
runtime/test/ompt/loadtool/tool.c
1 ↗(On Diff #119359)

Without a RUN line, lit complains about the missing RUN line. The lit summary lists the test as UNRESOLVED test.

So, probably merging the two files is the best approach.

Addressing Jonas' comments

protze.joachim marked 19 inline comments as done.Oct 18 2017, 8:03 AM
protze.joachim added inline comments.
runtime/src/kmp_tasking.cpp
1700–1705 ↗(On Diff #119359)

The event for this function was identified to incur serious overhead for benchmarks like nqueens/kdtree/fibonacci with if(0) cut-off. Hansang provided this implementation as a performance fix.

hbae added inline comments.Oct 18 2017, 8:17 AM
runtime/src/kmp_tasking.cpp
1692 ↗(On Diff #119487)

I know this is ugly, but we couldn't find a better way to fix ~15% regression in 376.kdtree.

Hahnfeld added inline comments.Oct 18 2017, 8:26 AM
runtime/src/kmp_tasking.cpp
1692 ↗(On Diff #119487)

Duplicating code increases the risk of inconsistencies. Can we use templates so that the compiler can do the work? Something like the following:

template <bool ompt>
kmp_int32 __kmpc_omp_taskwait_temp(ident_t *loc_ref, kmp_int32 gtid) {
  ...
}

kmp_int32 __kmpc_omp_taskwait(ident_t *loc_ref, kmp_int32 gtid) {
#if OMPT_SUPPORT && OMPT_OPTIONAL
  if (UNLIKELY(ompt_enabled.enabled)) {
    return __kmpc_omp_taskwait_temp<true>(loc_ref, gtid);
  }
#endif
  return __kmpc_omp_taskwait_temp<false>(loc_ref, gtid);
}
hbae added inline comments.Oct 18 2017, 9:49 AM
runtime/src/kmp_tasking.cpp
1692 ↗(On Diff #119487)

Yes, we can try this, but I suggest to do it in a separate patch since it requires performance validation.

Hahnfeld added inline comments.Oct 18 2017, 12:22 PM
runtime/src/kmp_tasking.cpp
1692 ↗(On Diff #119487)

But this solution is a maintenance nightmare. If we need time to do performance tests, then the "unoptimized" version should go in at first. So only one function that has all the instrumentation in it

Hahnfeld added inline comments.Oct 18 2017, 6:02 PM
runtime/src/kmp_tasking.cpp
1692 ↗(On Diff #119487)

For reference: In some offline tests with Joachim, I was able to convince the compiler to generate the exact same code. Slightly modified sketch that allows inlining of version without OMPT:

template <bool ompt>
static kmp_int32 __kmpc_omp_taskwait_temp(ident_t *loc_ref, kmp_int32 gtid, void *frame_address, void *return_address) {
  ...
}

OMPT_NOINLINE
static kmp_int32 __kmpc_omp_taskwait_ompt(ident *loc_ref, kmp_int32 gtid, void *frame_address, void *return_address) {
  return __kmpc_omp_taskwait_temp<true>(loc_ref, gtid, frame_address, return_address);
}

kmp_int32 __kmpc_omp_taskwait(ident_t *loc_ref, kmp_int32 gtid) {
#if OMPT_SUPPORT && OMPT_OPTIONAL
  if (UNLIKELY(ompt_enabled.enabled)) {
    return __kmpc_omp_taskwait_temp<true>(loc_ref, gtid, OMPT_GET_FRAME_ADDRESS(1), OMPT_GET_RETURN_ADDRESS(0));
  }
#endif
  return __kmpc_omp_taskwait_temp<false>(loc_ref, gtid, NULL, NULL);
}

Clang 5.0.0 even optimizes out the two NULL parameters.

protze.joachim added inline comments.Oct 19 2017, 2:08 AM
runtime/src/kmp_tasking.cpp
1692 ↗(On Diff #119487)

I agree with Jonas, that this code duplication is a maintenance nightmare. Looking at the object dump, the latest version looks very promising.

We will push an implementation of this modification (also applied for the task_if0 functions) to a new branch of the OMPT github repository later this week. This way you will have the chance to run performance tests.

hbae added inline comments.Oct 19 2017, 6:36 AM
runtime/src/kmp_tasking.cpp
1692 ↗(On Diff #119487)

Sounds good to me.

protze.joachim added inline comments.Oct 23 2017, 5:20 AM
runtime/src/kmp_tasking.cpp
1692 ↗(On Diff #119487)

The modifications are available at:
https://github.com/OpenMPToolsInterface/LLVM-openmp/tree/template-versioning-inline

Comparison of objects dumps for current implementation and templated implementation shows no changes in instructions when built with clang compiler (ignoring changed addresses/names). Therefore I tend to update this patch.

protze.joachim updated this revision to Diff 120365.EditedOct 26 2017, 1:01 AM

The latest update replaces things like "REQUIRES taskgroup" by explicit "XFAIL gcc" for testcases that are expected to fail because either gcc or the gomp interface of libomp don't support the construct or the construct has no runtime call. (see D38880)

Replaced the two function versions of __kmpc_omp_task_begin_if0, __kmpc_omp_task_complete_if0, __kmpc_omp_taskwait by a templated implementation.

hbae added inline comments.Oct 26 2017, 7:01 AM
runtime/src/kmp_tasking.cpp
569 ↗(On Diff #120365)

Return type is void.

591 ↗(On Diff #120365)

Return type is void.

1707 ↗(On Diff #120365)

I think the intention here was to call the outlined function __kmpc_omp_taskwait_ompt.

I think we are getting there :-) however, the current base for the diff looks old, can you update to the changes to .clang-format by @jlpeyton?

runtime/src/kmp_tasking.cpp
1707 ↗(On Diff #120365)

Oops, I must have missed that when I pushed my experimental changes to our internal GitLab...

runtime/src/ompt-general.cpp
12 ↗(On Diff #120365)

Do we need this include? D39182 removes it again

124 ↗(On Diff #120365)

next_tool = (ompt_start_tool_t)dlsym(RTLD_NEXT, "ompt_start_tool"); looks much more readable IMO

Since prerequisites are already in master, now the diff against master. (As Jonas pointed out, the last diff missed the clang-format in the base)

Fixed the return of void. Run clang-format again.

protze.joachim marked 2 inline comments as done.

Avoid inlining for the ompt_enabled case as Hansang pointed out

protze.joachim marked 23 inline comments as done.Oct 27 2017, 1:28 AM

I think I went over all comments and marked all that are not done yet.

One additional thing that should be doable with a global search-and-replace: This patch uses if (ompt.enabled), __builtin_expect and UNLIKELY inconsistently. At least for the latter two, we should agree on one. I prefer the macro because this makes it easier to no-op should there be a platform that doesn't support __builtin_expect (for whatever reason)

runtime/src/kmp_gsupport.cpp
1233–1234 ↗(On Diff #120554)

OMPT_STORE_RETURN_ADDRESS has if (ompt_enabled.enabled) built-in

runtime/src/kmp_tasking.cpp
602 ↗(On Diff #116382)

That's not done...

894 ↗(On Diff #116382)

Not done yet

runtime/src/ompt-general.cpp
124 ↗(On Diff #120365)

ping

57–60 ↗(On Diff #120554)

I think I asked this some days ago: Aren't these defined in ompt.h?

EDIT: Ah, in the TR6 patch!

219 ↗(On Diff #120554)

Hasn't this been parsed to __kmp_tool_libraries?

228 ↗(On Diff #120554)

start_tool = (ompt_start_tool_t)dlsym(RTLD_NEXT, "ompt_start_tool"); as above

runtime/src/ompt-internal.h
34 ↗(On Diff #120554)

Why is this here?

protze.joachim marked 3 inline comments as done.

Addressing the last remaining comments.

protze.joachim marked 3 inline comments as done.Oct 27 2017, 7:15 AM
hbae added inline comments.Oct 27 2017, 7:31 AM
runtime/src/ompt-general.cpp
124 ↗(On Diff #120365)

I am fine with the change.

219 ↗(On Diff #120554)

__kmp_tool_libraries is defined when the runtime reads environment variables, but ompt_pre_init() is called before that happens, so I think __kmp_tool_libraries is just used for printing the values of OMP_TOOL_LIBRARIES.

228 ↗(On Diff #120554)

Looks good to me as above.

protze.joachim marked 7 inline comments as done.

Addressing Jonas latest comments.

runtime/src/ompt-general.cpp
219 ↗(On Diff #120554)

This function is reached before __kmp_env_initialize, so before __kmp_tool_librariesis set,

runtime/src/ompt-internal.h
34 ↗(On Diff #120554)

Below in line 47, this is used to store the pointer for the scheduling task, which is needed to return the frame information.

protze.joachim added inline comments.Oct 27 2017, 7:43 AM
runtime/src/kmp_tasking.cpp
894 ↗(On Diff #116382)

Just changed this locally, waiting for comments before updating the diff again,

Two minor comments and UNLIKELY vs __builtin_expect

runtime/src/kmp_tasking.cpp
894 ↗(On Diff #116382)

Still

runtime/src/ompt-internal.h
34 ↗(On Diff #120554)

Sure, shouldn't this live in kmp.h is it's not already there?

hbae added a comment.Oct 27 2017, 7:49 AM

I also prefer UNLIKELY over __builtin_expect.

protze.joachim marked 4 inline comments as done.Oct 27 2017, 7:53 AM
protze.joachim added inline comments.
runtime/src/ompt-internal.h
34 ↗(On Diff #120554)

Do you suggest to move all ompt_*_info_t to kmp.h?

Or should I just move the include of ompt-internal.h after the forward declarations in kmp.h?

Hahnfeld added inline comments.Oct 27 2017, 8:03 AM
runtime/src/ompt-internal.h
34 ↗(On Diff #120554)

Ah, now I understand why you need this here, didn't get this before.

In this case, I'm fine with this (please add a comment that this file is included before the forward declarations) or you could replace the single use of kmp_taskdata_t with struct kmp_taskdata...

Remove the forward declaration in ompt-internal.h.
Remove unnecessary includes of ompt-internal.h.
Replace all _builtin_expect by the UNLIKELY macro.

protze.joachim marked 6 inline comments as done.Oct 27 2017, 8:14 AM

LGTM. Sorry for my continued nit-picking but I think a patch of this size deserves good reviews

hbae accepted this revision.Oct 30 2017, 6:59 AM
This revision is now accepted and ready to land.Oct 30 2017, 6:59 AM
Hahnfeld accepted this revision.Oct 30 2017, 7:06 AM

@protze.joachim have you been granted commit access?

This revision was automatically updated to reflect the committed changes.

I believe this broke some tests on Windows (full output below). The failures look pretty basic, like not finding pthread.h. Does the runtime not have test coverage on buildbots?

I'll disable openmp on windows for the 6.0 release and snapshots in the meantime.

[1/2] Running all regression tests
llvm-lit.py: C:/src/llvm\utils\lit\lit\llvm\config.py:334: note: using clang: c:\src\llvm\build.release\bin\clang.EXE
llvm-lit.py: C:/src/llvm/build.release/utils/lit/tests/lit.cfg:61: warning: Could not import psutil. Some tests will be skipped and the --timeout command line argument will not work.
-- Testing: 38733 tests, 32 threads --
Testing: 0 .. 10.. 20.. 30.. 40.. 50.. 60.. 70.. 80.. 90
FAIL: libomp :: ompt/misc/control_tool_no_ompt_support.c (37124 of 38733)
******************** TEST 'libomp :: ompt/misc/control_tool_no_ompt_support.c' FAILED ********************
Script:
--
C:/src/llvm/build.release/./bin/clang.exe -fopenmp  -I C:/src/llvm/projects/openmp/runtime/test -I C:/src/llvm/build.release/projects/openmp/runtime/src -L C:/src/llvm/build.release/bin  C:\src\llvm\projects\openmp\runtime\test\ompt\misc\control_tool_no_ompt_support.c -o C:\src\llvm\build.release\projects\openmp\runtime\test\ompt\misc\Output\control_tool_no_ompt_support.c.tmp && C:\src\llvm\build.release\projects\openmp\runtime\test\ompt\misc\Output\control_tool_no_ompt_support.c.tmp
--
Exit Code: 1120

Command Output (stdout):
--
$ "C:/src/llvm/build.release/./bin/clang.exe" "-fopenmp" "-I" "C:/src/llvm/projects/openmp/runtime/test" "-I" "C:/src/llvm/build.release/projects/openmp/runtime/src" "-L" "C:/src/llvm/build.release/bin" "C:\src\llvm\projects\openmp\runtime\test\ompt\misc\control_tool_no_ompt_support.c" "-o" "C:\src\llvm\build.release\projects\openmp\runtime\test\ompt\misc\Output\control_tool_no_ompt_support.c.tmp"
# command output:
control_tool_no_ompt_support-3fc04d.o : error LNK2019: unresolved external symbol omp_control_tool referenced in function .omp_outlined.
C:\src\llvm\build.release\projects\openmp\runtime\test\ompt\misc\Output\control_tool_no_ompt_support.c.tmp : fatal error LNK1120: 1 unresolved externals

# command stderr:
clang.exe: error: linker command failed with exit code 1120 (use -v to see invocation)

error: command failed with exit status: 1120

--

********************
Testing: 0 .. 10.. 20.. 30.. 40.. 50.. 60.. 70.. 80.. 90
FAIL: libomp :: misc_bugs/omp_foreign_thread_team_reuse.c (37143 of 38733)
******************** TEST 'libomp :: misc_bugs/omp_foreign_thread_team_reuse.c' FAILED ********************
Script:
--
C:/src/llvm/build.release/./bin/clang.exe -fopenmp  -I C:/src/llvm/projects/openmp/runtime/test -I C:/src/llvm/build.release/projects/openmp/runtime/src -L C:/src/llvm/build.release/bin  C:\src\llvm\projects\openmp\runtime\test\misc_bugs\omp_foreign_thread_team_reuse.c -o C:\src\llvm\build.release\projects\openmp\runtime\test\misc_bugs\Output\omp_foreign_thread_team_reuse.c.tmp -lpthread && C:\src\llvm\build.release\projects\openmp\runtime\test\misc_bugs\Output\omp_foreign_thread_team_reuse.c.tmp
--
Exit Code: 1181

Command Output (stdout):
--
$ "C:/src/llvm/build.release/./bin/clang.exe" "-fopenmp" "-I" "C:/src/llvm/projects/openmp/runtime/test" "-I" "C:/src/llvm/build.release/projects/openmp/runtime/src" "-L" "C:/src/llvm/build.release/bin" "C:\src\llvm\projects\openmp\runtime\test\misc_bugs\omp_foreign_thread_team_reuse.c" "-o" "C:\src\llvm\build.release\projects\openmp\runtime\test\misc_bugs\Output\omp_foreign_thread_team_reuse.c.tmp" "-lpthread"
# command output:
LINK : fatal error LNK1181: cannot open input file 'pthread.lib'

# command stderr:
In file included from C:\src\llvm\projects\openmp\runtime\test\misc_bugs\omp_foreign_thread_team_reuse.c:3:
C:/src/llvm/projects/openmp/runtime/test\omp_testsuite.h:53:60: warning: format specifies type 'unsigned int' but the argument has type 'DWORD' (aka 'unsigned long') [-Wformat]
    fprintf(stderr, "CreateThread() failed: Error #%u.\n", GetLastError());
                                                   ~~      ^~~~~~~~~~~~~~
                                                   %lu
C:/src/llvm/projects/openmp/runtime/test\omp_testsuite.h:65:13: warning: format specifies type 'unsigned int' but the argument has type 'DWORD' (aka 'unsigned long') [-Wformat]
            GetLastError());
            ^~~~~~~~~~~~~~
C:/src/llvm/projects/openmp/runtime/test\omp_testsuite.h:70:59: warning: format specifies type 'unsigned int' but the argument has type 'DWORD' (aka 'unsigned long') [-Wformat]
    fprintf(stderr, "CloseHandle() failed: Error #%u.\n", GetLastError());
                                                  ~~      ^~~~~~~~~~~~~~
                                                  %lu
C:\src\llvm\projects\openmp\runtime\test\misc_bugs\omp_foreign_thread_team_reuse.c:40:1: warning: control reaches end of non-void function [-Wreturn-type]
}
^
4 warnings generated.
clang.exe: error: linker command failed with exit code 1181 (use -v to see invocation)

error: command failed with exit status: 1181

--

********************
Testing: 0 .. 10.. 20.. 30.. 40.. 50.. 60.. 70.. 80.. 90
FAIL: libomp :: tasking/bug_nested_proxy_task.c (37154 of 38733)
******************** TEST 'libomp :: tasking/bug_nested_proxy_task.c' FAILED ********************
Script:
--
C:/src/llvm/build.release/./bin/clang.exe -fopenmp  -I C:/src/llvm/projects/openmp/runtime/test -I C:/src/llvm/build.release/projects/openmp/runtime/src -L C:/src/llvm/build.release/bin  C:\src\llvm\projects\openmp\runtime\test\tasking\bug_nested_proxy_task.c -o C:\src\llvm\build.release\projects\openmp\runtime\test\tasking\Output\bug_nested_proxy_task.c.tmp -lpthread && C:\src\llvm\build.release\projects\openmp\runtime\test\tasking\Output\bug_nested_proxy_task.c.tmp
--
Exit Code: 1

Command Output (stdout):
--
$ "C:/src/llvm/build.release/./bin/clang.exe" "-fopenmp" "-I" "C:/src/llvm/projects/openmp/runtime/test" "-I" "C:/src/llvm/build.release/projects/openmp/runtime/src" "-L" "C:/src/llvm/build.release/bin" "C:\src\llvm\projects\openmp\runtime\test\tasking\bug_nested_proxy_task.c" "-o" "C:\src\llvm\build.release\projects\openmp\runtime\test\tasking\Output\bug_nested_proxy_task.c.tmp" "-lpthread"
# command stderr:
C:\src\llvm\projects\openmp\runtime\test\tasking\bug_nested_proxy_task.c:7:10: fatal error: 'pthread.h' file not found
#include <pthread.h>
         ^~~~~~~~~~~
1 error generated.

error: command failed with exit status: 1

--

********************
Testing: 0 .. 10.. 20.. 30.. 40.. 50.. 60.. 70.. 80.. 90
FAIL: libomp :: tasking/bug_proxy_task_dep_waiting.c (37156 of 38733)
******************** TEST 'libomp :: tasking/bug_proxy_task_dep_waiting.c' FAILED ********************
Script:
--
C:/src/llvm/build.release/./bin/clang.exe -fopenmp  -I C:/src/llvm/projects/openmp/runtime/test -I C:/src/llvm/build.release/projects/openmp/runtime/src -L C:/src/llvm/build.release/bin  C:\src\llvm\projects\openmp\runtime\test\tasking\bug_proxy_task_dep_waiting.c -o C:\src\llvm\build.release\projects\openmp\runtime\test\tasking\Output\bug_proxy_task_dep_waiting.c.tmp -lpthread && C:\src\llvm\build.release\projects\openmp\runtime\test\tasking\Output\bug_proxy_task_dep_waiting.c.tmp
--
Exit Code: 1

Command Output (stdout):
--
$ "C:/src/llvm/build.release/./bin/clang.exe" "-fopenmp" "-I" "C:/src/llvm/projects/openmp/runtime/test" "-I" "C:/src/llvm/build.release/projects/openmp/runtime/src" "-L" "C:/src/llvm/build.release/bin" "C:\src\llvm\projects\openmp\runtime\test\tasking\bug_proxy_task_dep_waiting.c" "-o" "C:\src\llvm\build.release\projects\openmp\runtime\test\tasking\Output\bug_proxy_task_dep_waiting.c.tmp" "-lpthread"
# command stderr:
C:\src\llvm\projects\openmp\runtime\test\tasking\bug_proxy_task_dep_waiting.c:7:10: fatal error: 'pthread.h' file not found
#include <pthread.h>
         ^~~~~~~~~~~
1 error generated.

error: command failed with exit status: 1

--

********************
Testing: 0 .. 10.. 20.. 30.. 40.. 50.. 60.. 70.. 80.. 90..
Testing Time: 269.12s
********************
Failing Tests (4):
    libomp :: misc_bugs/omp_foreign_thread_team_reuse.c
    libomp :: ompt/misc/control_tool_no_ompt_support.c
    libomp :: tasking/bug_nested_proxy_task.c
    libomp :: tasking/bug_proxy_task_dep_waiting.c

  Expected Passes    : 37046
  Expected Failures  : 243
  Unsupported Tests  : 1440
  Unexpected Failures: 4
hans added a comment.Jan 23 2018, 7:03 AM

Also, r317085 doesn't seem to have gone to any mailing list, which is unfortunate. Maybe because the committer was not subscribed and it got stuck in moderation.

In D38185#985047, @hans wrote:

Also, r317085 doesn't seem to have gone to any mailing list, which is unfortunate. Maybe because the committer was not subscribed and it got stuck in moderation.

I think at the time of the commit, I was not subscribed with the matching mail address :( And I guess the size of the diff might also be a reason for the mailing list to put the mail on wait.

Of the failing tests, we only added one with the OMPT commit:

libomp :: misc_bugs/omp_foreign_thread_team_reuse.c

https://reviews.llvm.org/D23084

libomp :: ompt/misc/control_tool_no_ompt_support.c

by this commit.

libomp :: tasking/bug_nested_proxy_task.c

https://reviews.llvm.org/D23115

libomp :: tasking/bug_proxy_task_dep_waiting.c

https://reviews.llvm.org/D23116

I uploaded a diff, that should address all the broken test cases:
https://reviews.llvm.org/D42427

hans added a comment.Jan 23 2018, 9:02 AM
In D38185#985047, @hans wrote:

Also, r317085 doesn't seem to have gone to any mailing list, which is unfortunate. Maybe because the committer was not subscribed and it got stuck in moderation.

I think at the time of the commit, I was not subscribed with the matching mail address :( And I guess the size of the diff might also be a reason for the mailing list to put the mail on wait.

Of the failing tests, we only added one with the OMPT commit:

libomp :: misc_bugs/omp_foreign_thread_team_reuse.c

https://reviews.llvm.org/D23084

libomp :: ompt/misc/control_tool_no_ompt_support.c

by this commit.

libomp :: tasking/bug_nested_proxy_task.c

https://reviews.llvm.org/D23115

libomp :: tasking/bug_proxy_task_dep_waiting.c

https://reviews.llvm.org/D23116

I uploaded a diff, that should address all the broken test cases:
https://reviews.llvm.org/D42427

Thanks!

openmp/trunk/runtime/src/ompt-general.cpp
118

This fails on Windows obviously. When building with clang it looks like this:

[903/3933] Building CXX object projects\openmp\runtime\src\CMakeFiles\omp.dir\ompt-general.cpp.obj
FAILED: projects/openmp/runtime/src/CMakeFiles/omp.dir/ompt-general.cpp.obj
\src\llvm\build.release\bin\clang-cl.exe  /nologo -TP -D_CRT_NONSTDC_NO_DEPRECATE -D_CRT_NONSTDC_NO_WARNINGS -D_CRT_SECURE_NO_DEPRECATE -D_CRT_SECURE_NO_WARNINGS -D_DEBUG_POINTER_IMPL="" -D_SCL_SECURE_NO_DEPRECATE -D_SCL_SECURE_NO_WARNINGS -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Domp_EXPORTS -Iprojects\openmp\runtime\src -I..\projects\openmp\runtime\src -Iinclude -I..\include -I..\projects\openmp\runtime\src\i18n -I..\projects\openmp\runtime\src\include\50 -I..\projects\openmp\runtime\src\thirdparty\ittnotify /DWIN32 /D_WINDOWS   /Zc:inline /Zc:strictStrings /Oi /Zc:rvalueCast /Brepro /W4  -W -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -Wcovered-switch-default -Wdelete-non-virtual-dtor -Wstring-conversion /MT /O2 /Ob2   -UNDEBUG -D _CRT_SECURE_NO_WARNINGS -D _CRT_SECURE_NO_DEPRECATE -D _WINDOWS -D _WINNT -D _WIN32_WINNT=0x0501 -D _USRDLL -Wno-sign-compare -Wno-unused-function -Wno-unused-local-typedef -Wno-unused-value -Wno-unused-variable -Wno-switch -Wno-covered-switch-default -Wno-deprecated-register -Wno-gnu-anonymous-struct -Wno-unknown-pragmas -Wno-missing-field-initializers -Wno-missing-braces -Wno-comment -Wno-self-assign -Wno-vla-extension -Wno-format-pedantic /GS /EHsc /showIncludes /Foprojects\openmp\runtime\src\CMakeFiles\omp.dir\ompt-general.cpp.obj /Fdprojects\openmp\runtime\src\CMakeFiles\omp.dir\ -c ..\projects\openmp\runtime\src\ompt-general.cpp
..\projects\openmp\runtime\src\ompt-general.cpp(131,32):  error: use of undeclared identifier 'RTLD_NEXT'
      (ompt_start_tool_t)dlsym(RTLD_NEXT, "ompt_start_tool");
                               ^

But for some reason I don't see that when building with MSVC.

Of the failing tests, we only added one with the OMPT commit:

libomp :: misc_bugs/omp_foreign_thread_team_reuse.c

https://reviews.llvm.org/D23084

libomp :: ompt/misc/control_tool_no_ompt_support.c

by this commit.

libomp :: tasking/bug_nested_proxy_task.c

https://reviews.llvm.org/D23115

libomp :: tasking/bug_proxy_task_dep_waiting.c

https://reviews.llvm.org/D23116

Except for the OMPT test all of those were in August 2016 (!) and should be in 4.0 and 5.0...

Does the runtime not have test coverage on buildbots?

Not on Windows and not as good as I would like it to be on Linux. We (IT Center, RWTH Aachen University) are looking into improving the latter. Meanwhile, Intel is saying that they do extensive testing of the runtime - whatever that means on Windows...

Hahnfeld added inline comments.Jan 23 2018, 9:07 AM
openmp/trunk/runtime/src/ompt-general.cpp
118

Because it's #if KMP_OS_UNIX. If that's not set with clang-cl on Windows then there is definitely something wrong!

Intel is saying that they do extensive testing of the runtime - whatever that means on Windows...

Currently we have limited testing of OMPT features on Windows.