[Openmp-commits] [PATCH] D100181: [OpenMP] [OMPD] [1/6] Implementation of OMPD debugging library - libompd. Code changes in openmp/runtime to support libompd.

[Joachim Protze via Phabricator via Openmp-commits]

protze.joachim added inline comments.


================
Comment at: openmp/runtime/src/kmp_runtime.cpp:2050
     propagateFPControl(team);
+#if OMPD_SUPPORT
+    if (ompd_state & OMPD_ENABLE_BP)
----------------
jini.susan wrote:
> hbae wrote:
> > We already have program points defined for OMPT callbacks.
> > Isn't it better to place parallel begin/end BP at the places near OMPT parallel begin/end?
> > Are there any missing parallel begin/end not covered by the current OMPT parallel begin/end?
> According to the spec, there is a difference in the binding associated with the current parallel handle with the OMPT parallel-begin event and the OMPD parallel-begin event. 
> 
> For OMPD, (Section 5.6.1.)
> 
> The OpenMP implementation must execute ompd_bp_parallel_begin at every
> parallel-begin event. At the point that the implementation reaches
> ompd_bp_parallel_begin, the binding for ompd_get_curr_parallel_handle is the
> parallel region that is beginning and the binding for ompd_get_curr_task_handle is the
> task that encountered the parallel construct.
> 
> For OMPT, 
> 
> Between a parallel-begin event and an implicit-task-begin event, a call to
> ompt_get_parallel_info(0,...) may return information about the outer parallel team,
> the new parallel team or an inconsistent state.
> 
> The OMPD runtime entry points in this implementation are invoked at points to ensure that the right binding holds as per the spec.
> 
The OMPT parallel-begin is placed before the runtime gets a lock for creating the team. All information provided by the callback is available at this point. Other information like the number of threads is passed in the implicit-task-begin event.
As Jini pointed out, there is not enough information available for OMPD parallel-begin at that point.


================
Comment at: openmp/runtime/src/ompd-specific.h:126-127
+
+// TODO: (mr) this is a hack to cast cuda contexts to 64 bit values
+typedef uint64_t ompd_cuda_context_ptr_t;
+
----------------
Please drop these lines. The current patches don't include any cuda stuff.


================
Comment at: openmp/runtime/src/ompd-specific.h:156
+  OMPD_SIZEOF(__kmp_nth)                                                       \
+  OMPD_SIZEOF(ompd_cuda_context_ptr_t)
+
----------------
same here, drop the cuda symbol


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100181/new/

https://reviews.llvm.org/D100181