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

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
1233–1234

The comment is not relevant any more

2541

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

runtime/src/kmp_global.cpp
306

LIBOMP_OMPT_SUPPORT -> OMPT_SUPPORT

runtime/src/kmp_gsupport.cpp
34

Should be under OMPT_OPTIONAL?

43

Should be under OMPT_OPTIONAL?

65

Should be under OMPT_OPTIONAL?

75

Should be under OMPT_OPTIONAL?

181

Should be under OMPT_OPTIONAL?

195

Should be under OMPT_OPTIONAL?

218

Should be under OMPT_OPTIONAL and more others below?

254

Should be under OMPT_OPTIONAL?

642

Should be under OMPT_OPTIONAL?

830

Should be under OMPT_OPTIONAL?

omalyshe added inline comments.Oct 3 2017, 8:43 AM
runtime/src/kmp_sched.cpp
142–146

Remove the comment

194–198

Remove the comment

223–227

Remove the comment

380–384

Remove the comment

runtime/src/kmp_settings.cpp
4355–4377

LIBOMP_OMPT_SUPPORT -> OMPT_SUPPORT

4622

LIBOMP_OMPT_SUPPORT -> OMPT_SUPPORT

runtime/src/kmp_tasking.cpp
602

Remove extra empty line

893

to be removed

898

to be removed

1598–1602

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
28

This is not necessary since we have omp_control_tool now.

runtime/src/include/50/ompt.h.var
679–680

Do we still need this function?

runtime/src/kmp_csupport.cpp
2984

Yes, it should be __ompt_get_mutex_impl_type(user_lock)

runtime/src/kmp_tasking.cpp
456

ompt_tool -> ompt_start_tool

460

Remove the commented if condition.

598

task_data is never used.

omalyshe added inline comments.Oct 4 2017, 7:14 AM
runtime/src/kmp_barrier.cpp
1889

OMPT_CUR_TASK_DATA(this_thr) is defined for this.

runtime/src/kmp_csupport.cpp
486

OMPT_CUR_TASK_INFO () to be used

491

OMPT_CUR_TASK_DATA(this_thr) is defined for this

runtime/src/kmp_ftn_entry.h
359

OMPT_CUR_TASK_INFO() to be used

runtime/src/kmp_runtime.cpp
1209

OMPT_CUR_TASK_INFO() to be used

1402

OMPT_CUR_TASK_DATA(this_thr) is defined for this

7215

OMPT_CUR_TASK_DATA(this_thr) is defined for this

7216

OMPT_CUR_TEAM_DATA to be used

7223

OMPT_CUR_TEAM_INFO() to be used

runtime/src/kmp_tasking.cpp
1618

OMPT_CUR_TEAM_DATA() to be used

runtime/src/kmp_wait_release.h
112

What is pteam?

224–234

OMPT_CUR_TEAM_DATA(this_thr) to be used

225

OMPT_CUR_TASK_DATA(this_thr) to be used

runtime/src/ompt-specific.cpp
267

OMPT_CUR_TEAM_INFO(thr) to be used

268

OMPT_CUR_TEAM_INFO(thr) to be used

272

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

282

OMPT_CUR_TEAM_INFO(thr) to be used

283

OMPT_CUR_TASK_INFO() to be used

294

OMPT_CUR_TEAM_INFO(thr) to be used

299

OMPT_CUR_TASK_INFO() to be used

runtime/src/ompt-specific.h
56

never used

58

never used

60

never used

62

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

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

482

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
123–125

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

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

1287

Remove this as well for the same reason

1359

And remove this too.

Addressed latest comments by jlpeyton

runtime/src/kmp_ftn_os.h
54

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

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

419–421

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

Nit: This commit id is probably in the fork.

runtime/src/include/50/ompt.h.var
224

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

runtime/src/kmp_gsupport.cpp
155–156

Remove

940

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

1206–1211

There is IF_OMPT_SUPPORT above, please collapse

runtime/src/kmp_runtime.cpp
1100–1105

This formatting looks weird?

1208–1212

Remove commented coe

1215–1216

Remove

1223–1224

Remove

1483

Remove

1515–1516

Remove

5946–5948

Formatting looks weird

runtime/src/kmp_tasking.cpp
492–494

__ompt_task_finish?

Hahnfeld added inline comments.Oct 17 2017, 5:10 PM
runtime/src/kmp_tasking.cpp
496–497

Remove this code if that's guaranteed

1469–1470

Remove

1700–1705

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

runtime/src/ompt-event-specific.h
25–28

Remove

runtime/test/ompt/callback.h
70–92

Remove

115–130

Remove

runtime/test/ompt/cancel/cancel_parallel.c
16–19

There is a macro for this!

runtime/test/ompt/loadtool/tool.c
2

That's not really a test...

runtime/test/ompt/synchronization/taskwait.c
35–38

Why are these deactivated?

runtime/test/ompt/tasks/task_types.c
112–113

Deactivated?

omalyshe added inline comments.Oct 18 2017, 1:44 AM
runtime/src/kmp_gsupport.cpp
940

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

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
2

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
2

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
2

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

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
1705

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
1705

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
1705

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
1705

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
1705

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
1705

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
1705

Sounds good to me.

protze.joachim added inline comments.Oct 23 2017, 5:20 AM
runtime/src/kmp_tasking.cpp
1705

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
612

Return type is void.

634

Return type is void.

1785

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
1785

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

runtime/src/ompt-general.cpp
12

Do we need this include? D39182 removes it again

124

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
1237–1238

OMPT_STORE_RETURN_ADDRESS has if (ompt_enabled.enabled) built-in

runtime/src/kmp_tasking.cpp
602

That's not done...

893

Not done yet

runtime/src/ompt-general.cpp
58–62

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

EDIT: Ah, in the TR6 patch!

124

ping

220

Hasn't this been parsed to __kmp_tool_libraries?

229

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

runtime/src/ompt-internal.h
34

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

I am fine with the change.

220

__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.

229

Looks good to me as above.

protze.joachim marked 7 inline comments as done.

Addressing Jonas latest comments.

runtime/src/ompt-general.cpp
220

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

runtime/src/ompt-internal.h
34

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
893

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
893

Still

runtime/src/ompt-internal.h
34

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

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

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 ↗(On Diff #121108)

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 ↗(On Diff #121108)

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.