This is an archive of the discontinued LLVM Phabricator instance.

[OMPT] Fix assertion for OpenMP code generated with outdated compilers
ClosedPublic

Authored by protze.joachim on Nov 10 2017, 1:48 AM.

Details

Summary

For up-to-date compilers, this assertion is reasonable, but it breaks compatibility with the typical compiler installed on most systems.
This patch changes the default value to what we had when there was no compiler support. A warning about the outdated compiler is printed during runtime, when this point is reached.

Alternative solution:
Set a configure flag that explicitly asks for backwards compatibility and leave the assert there.

Diff Detail

Repository
rL LLVM

Event Timeline

protze.joachim created this revision.Nov 10 2017, 1:48 AM
omalyshe added inline comments.Nov 10 2017, 5:00 AM
runtime/src/kmp_csupport.cpp
1777 ↗(On Diff #122403)

I'd suggest using KMP_WARNING to emit standard RTL warning.
The message (in the message catalog) could be the following: "Cannot determine workshare type; using the default (loop) instead".

Also, it would be enough if only one thread emitted the warning.

Hahnfeld edited edge metadata.Nov 10 2017, 5:07 AM

To make the tests work with older compilers we need to reliably determine the compiler id and version. I'll be working on that.

runtime/src/kmp_csupport.cpp
1777 ↗(On Diff #122403)

In addition I think it would also be enough to diagnose in __kmp_for_static_init: The compiler will either generate the information for both calls or not at all.

Addressing Olgas suggestions

runtime/src/kmp_csupport.cpp
1777 ↗(On Diff #122403)

Is there a standard approach to only provide a single warning? Looking at other warnings, they are not filtered at all.

omalyshe added inline comments.Nov 10 2017, 6:12 AM
runtime/src/kmp_sched.cpp
85 ↗(On Diff #122428)

I think Jonas meant not to emit warning here; just silently set to the default value

omalyshe added inline comments.Nov 10 2017, 6:16 AM
runtime/src/kmp_sched.cpp
85 ↗(On Diff #122428)

Sorry, this comment is about *_fini, of course.

Remove warning for fini

protze.joachim marked 3 inline comments as done.Nov 10 2017, 6:44 AM
omalyshe added inline comments.Nov 10 2017, 7:28 AM
runtime/src/kmp_sched.cpp
85 ↗(On Diff #122431)

For a single warning something like this should work

@@ -68,6 +68,10 @@ static void __kmp_for_static_init(ident_t *loc, kmp_int32 global_tid,
   ompt_task_info_t *task_info = NULL;
   ompt_work_type_t ompt_work_type;

+  static kmp_int8 warn = 0;
+
   if (ompt_enabled.enabled) {
     // Only fully initialize variables needed by OMPT if OMPT is enabled.
     team_info = __ompt_get_teaminfo(0, NULL);
@@ -81,8 +85,12 @@ static void __kmp_for_static_init(ident_t *loc, kmp_int32 global_tid,
       } else if ((loc->flags & KMP_IDENT_WORK_DISTRIBUTE) != 0) {
         ompt_work_type = ompt_work_distribute;
       } else {
-        KMP_ASSERT2(0,
-                    "__kmpc_for_static_init: can't determine workshare type"); 
+        ompt_work_type = ompt_work_loop;
+       kmp_int8 bool_res = KMP_COMPARE_AND_STORE_ACQ8(&warn, (kmp_int8)0, (kmp_int8)1);
+       if(bool_res)
+           KMP_WARNING(...)
       }

Using Olga's code to only trigger a single warning. Tested and works for me.

protze.joachim marked 3 inline comments as done.Nov 10 2017, 7:41 AM

Please run clang-format on the changes.

runtime/src/kmp_sched.cpp
69 ↗(On Diff #122435)

Maybe we should have the default ompt_work_loop go here, just in case loc == NULL? (same for _fini)

Fix potentially uninitialized value. Thanks, Jonas!

protze.joachim marked an inline comment as done.Nov 10 2017, 8:31 AM
hbae accepted this revision.Nov 10 2017, 11:32 AM

LGTM

This revision is now accepted and ready to land.Nov 10 2017, 11:32 AM
This revision was automatically updated to reflect the committed changes.