[Openmp-commits] [PATCH] D43308: [OMPT] Fix parallel_data in implicit barrier-end

[Joachim Protze via Phabricator via Openmp-commits]

protze.joachim requested changes to this revision.
protze.joachim added inline comments.
This revision now requires changes to proceed.


================
Comment at: runtime/src/kmp_barrier.cpp:1892
-                                : &(this_thr->th.ompt_thread_info.task_data);
-      this_thr->th.ompt_thread_info.state = omp_state_overhead;
 #if OMPT_OPTIONAL
----------------
Please keep that one and remove the duplikate at the end of the block, including the comment.

You can add a comment here, that this state change is important for the algorithm described in runtime/src/kmp_wait_release.h:161


================
Comment at: runtime/src/kmp_runtime.cpp:7171
-    ompt_data_t *pId = OMPT_CUR_TEAM_DATA(this_thr);
-    this_thr->th.ompt_thread_info.state = omp_state_overhead;
 #if OMPT_OPTIONAL
----------------
Same here:
Please keep that one and remove the duplikate at the end of the block, including the comment.

You can add a comment here, that this state change is important for the algorithm described in runtime/src/kmp_wait_release.h:161


================
Comment at: runtime/test/ompt/parallel/normal.c:16
   // Check if libomp supports the callbacks for this test.
-  // CHECK-NOT: {{^}}0: Could not register callback 'ompt_callback_thread_begin'
-  // CHECK-NOT: {{^}}0: Could not register callback 'ompt_callback_thread_end'
-  // CHECK-NOT: {{^}}0: Could not register callback 'ompt_callback_parallel_begin'
-  // CHECK-NOT: {{^}}0: Could not register callback 'ompt_callback_parallel_end'
-  // CHECK-NOT: {{^}}0: Could not register callback 'ompt_callback_implicit_task'
-
+  // CHECK-NOT: {{^}}0: Could not register callback
 
----------------
The idea of listing the individual callbacks here was to eventually allow most tests to run successfully with LIBOMP_OMPT_OPTIONAL=off.
But, I guess, this could also be achieved by not registering the optional callbacks in that case.


================
Comment at: runtime/test/ompt/parallel/normal.c:33-35
+  // Note that we don't check the arguments of the calllbacks because FileCheck
+  // doesn't support CHECK-DAG-SAME (these lines are silently ignored).
+  // Instead this is done with the THREADS prefix.
----------------
Hahnfeld wrote:
> Another option would be to only use `CHECK-DAG` but this doesn't enforce the requirement that the arguments are on the same line. IMO we don't lose any coverage because the same information is checked in `THREADS`.
> 
> To be honest, I even think we could drop the previous `CHECK-SAME` lines and only check that we get the right callbacks before `parallel_end`...
Only checking for the right sequence of events here sounds good to me.


Repository:
  rOMP OpenMP

https://reviews.llvm.org/D43308