[Openmp-commits] [PATCH] D12998: Overhaul OMPT initialization interface

[Jonathan Peyton via Openmp-commits]

jlpeyton requested changes to this revision.
jlpeyton added a comment.
This revision now requires changes to proceed.

See inline comments.


================
Comment at: runtime/src/kmp_runtime.c:6379-6389
@@ -6378,9 +6378,13 @@
 
+#if OMPT_SUPPORT
+    ompt_pre_init();
+#endif
+
     KA_TRACE( 10, ("__kmp_do_serial_initialize: enter\n" ) );
 
     KMP_DEBUG_ASSERT( sizeof( kmp_int32 ) == 4 );
     KMP_DEBUG_ASSERT( sizeof( kmp_uint32 ) == 4 );
     KMP_DEBUG_ASSERT( sizeof( kmp_int64 ) == 8 );
     KMP_DEBUG_ASSERT( sizeof( kmp_uint64 ) == 8 );
     KMP_DEBUG_ASSERT( sizeof( kmp_intptr_t ) == sizeof( void * ) );
 
----------------
Small nit, can you move this pre_init() call after the 5 KMP_DEBUG_ASSERT()'s?

================
Comment at: runtime/src/kmp_runtime.c:6631-6633
@@ -6626,5 +6630,5 @@
     KA_TRACE( 10, ("__kmp_do_serial_initialize: exit\n" ) );
 #if OMPT_SUPPORT
-    ompt_init();
+    ompt_post_init();
 #endif
 }
----------------
Small nit, can you move these three lines before the KMP_MB() above?

================
Comment at: runtime/src/kmp_runtime.c:6768-6771
@@ -6763,2 +6767,6 @@
 {
+#if OMPT_SUPPORT
+    ompt_pre_init();
+#endif
+
     if ( __kmp_init_middle ) {
----------------
This is unnecessary since middle_init() -> do_middle_init() -> serial_init() -> do_serial_init().  Plus, the ompt_[pre|post]_init()'s in the do_serial_init() are guarded by the __kmp_initz_lock.  The guarentee with all this init() code is that do_serial_init() will happen first and be executed safely by a single  thread.

================
Comment at: runtime/src/kmp_runtime.c:6792-6795
@@ -6783,2 +6791,6 @@
 
+#if OMPT_SUPPORT
+    ompt_pre_init();
+#endif
+
     /* syncronize parallel initialization (for sibling) */
----------------
Unnecessary because parallel_init() -> do_middle_init() and so on.

================
Comment at: runtime/src/kmp_runtime.c:6859
@@ -6846,3 +6858,3 @@
 #if OMPT_SUPPORT
-    ompt_init();
+    ompt_post_init();
 #endif
----------------
Unnecessary as well since this parallel_init() -> ... -> do_serial_init().

================
Comment at: runtime/src/ompt-general.c:55-59
@@ +54,7 @@
+
+typedef void (*ompt_initialize_t) (
+  ompt_function_lookup_t ompt_fn_lookup, 
+  const char *version,
+  unsigned int ompt_version
+);
+
----------------
In addition to the indention problem Jonas pointed out, have these be four spaces as well.

================
Comment at: runtime/src/ompt-general.c:115
@@ +114,3 @@
+    //--------------------------------------------------
+    // Use a tool iff a tool available and enabled.
+    //--------------------------------------------------
----------------
omalyshe wrote:
> typo "iff" ->"if"
Olga, this is short for "if and only if".  At least I believe that is what is meant here.


Repository:
  rL LLVM

http://reviews.llvm.org/D12998