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

[Jonas Hahnfeld via Phabricator via Openmp-commits]

Hahnfeld added inline comments.


================
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
----------------
protze.joachim wrote:
> 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
I'm not sure I understand your reasoning here: There is no intervening call to `__kmp_wait_template` inside this OMPT block so I don't think there is an observable difference except the state during the callback.


================
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.
----------------
protze.joachim wrote:
> 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.
Okay, I'll remove the other `CHECK-SAME`s as proposed.


Repository:
  rOMP OpenMP

https://reviews.llvm.org/D43308