[Openmp-commits] [PATCH] D38185: Implementation of OMPT as specified in OpenMP 5.0 Preview 1
Jonas Hahnfeld via Phabricator via Openmp-commits
openmp-commits at lists.llvm.org
Tue Oct 17 17:10:56 PDT 2017
Hahnfeld added a comment.
Some more comments, there is still //a lot// of commented code that should be removed.
Another, higher level question: When should frame addresses be set? Currently, that's inconsistently guarded with `OMPT_OPTIONAL`. However, if I remember correctly this is mandatory?
================
Comment at: runtime/CMakeLists.txt:324
"Trace OMPT initialization?")
+#after testing: turn on ompt support for OpenMP 5.0 and higher (see commit 5e848176)
set(LIBOMP_OMPT_SUPPORT FALSE CACHE BOOL
----------------
Nit: This commit id is probably in the fork.
================
Comment at: runtime/src/include/50/ompt.h.var:224
-typedef enum {
+typedef enum ompt_callbacks_e{
#define ompt_event_macro(event, callback, eventid) event = eventid,
----------------
This file might not have been formatted? There are 4 spaces instead of 2 further down...
================
Comment at: runtime/src/kmp_gsupport.cpp:155-156
+ }
+ // this_thr->th.ompt_thread_info.state =
+ // omp_state_work_parallel;
+ }
----------------
Remove
================
Comment at: runtime/src/kmp_gsupport.cpp:940
#endif
+ // TODO: Intel, why call kmpc and not kmp here?
__kmpc_omp_task(&loc, gtid, task);
----------------
Nothing to do with this patch but an interesting question...
================
Comment at: runtime/src/kmp_gsupport.cpp:1206-1211
+#if OMPT_SUPPORT
+#define INCLUDE_IF_OMPT_SUPPORT(code) code
+#else
+#define INCLUDE_IF_OMPT_SUPPORT(code)
+#endif
+
----------------
There is `IF_OMPT_SUPPORT` above, please collapse
================
Comment at: runtime/src/kmp_runtime.cpp:1100-1104
+ // from
+ // those we now have.
// If it is FALSE we didn't save anything yet, but our objective is the
- // same. We have to ensure that the values in the team are the same as those
- // we have.
+ // same. We
+ // have to ensure that the values in the team are the same as those we have.
----------------
This formatting looks weird?
================
Comment at: runtime/src/kmp_runtime.cpp:1208-1212
+ // if (serial_team->t.t_level > 1)
+ parent_task_info = OMPT_CUR_TASK_INFO(this_thr);
+ // else
+ // parent_task_info =
+ // &(this_thr->th.th_current_task->td_parent->ompt_task_info);
----------------
Remove commented coe
================
Comment at: runtime/src/kmp_runtime.cpp:1215-1216
+ parent_task_info->frame.reenter_runtime_frame = OMPT_GET_FRAME_ADDRESS(1);
+ // printf("281474976710657: %p frame\n",
+ // OMPT_CUR_TASK_INFO(this_thr)->frame.exit_runtime_frame);
+ if (ompt_enabled.ompt_callback_parallel_begin) {
----------------
Remove
================
Comment at: runtime/src/kmp_runtime.cpp:1223-1224
+ &ompt_parallel_data, team_size,
+ // master_set_numthreads ? master_set_numthreads : get__nproc_2(
+ // parent_team, master_tid ),
+ ompt_invoker_program, codeptr);
----------------
Remove
================
Comment at: runtime/src/kmp_runtime.cpp:1483
+ NULL, NULL);
+ // ompt_parallel_data = &(team->t.ompt_team_info.parallel_data);
+ return_address = OMPT_LOAD_RETURN_ADDRESS(gtid);
----------------
Remove
================
Comment at: runtime/src/kmp_runtime.cpp:1515-1516
+ parent_task_data, ompt_frame, &ompt_parallel_data, team_size,
+ // master_set_numthreads ? master_set_numthreads : get__nproc_2(
+ // parent_team, master_tid ),
+ OMPT_INVOKER(call_context), return_address);
----------------
Remove
================
Comment at: runtime/src/kmp_runtime.cpp:5946-5947
// __kmp_monitor will appear to contain valid data, but it is only valid in
- // the parent process, not the child.
+ // the
+ // parent process, not the child.
// New behavior (201008): instead of keying off of the flag
----------------
Formatting looks weird
================
Comment at: runtime/src/kmp_tasking.cpp:492-494
+// __ompt_task_start:
+// Build and trigger task-end event
+static inline void __ompt_task_finish(kmp_task_t *task,
----------------
`__ompt_task_finish`?
================
Comment at: runtime/src/kmp_tasking.cpp:496-497
+ kmp_taskdata_t *resumed_task) {
+ // The calls to __ompt_task_finish already have the ompt_enabled condition.
+ // if (__builtin_expect(ompt_enabled.enabled,0)){
+ kmp_taskdata_t *taskdata = KMP_TASK_TO_TASKDATA(task);
----------------
Remove this code if that's guaranteed
================
Comment at: runtime/src/kmp_tasking.cpp:1469-1470
+ parent = new_taskdata->td_parent;
+ // parent->ompt_task_info.frame.reenter_runtime_frame =
+ // OMPT_GET_FRAME_ADDRESS(1);
+ if (ompt_enabled.ompt_callback_task_create) {
----------------
Remove
================
Comment at: runtime/src/kmp_tasking.cpp:1700-1705
+kmp_int32 __kmpc_omp_taskwait(ident_t *loc_ref, kmp_int32 gtid) {
+#if OMPT_SUPPORT && OMPT_OPTIONAL
+ if (UNLIKELY(ompt_enabled.enabled)) {
+ return __ompt_enabled_taskwait(loc_ref, gtid, OMPT_GET_FRAME_ADDRESS(1),
+ OMPT_GET_RETURN_ADDRESS(0));
+ }
----------------
Puh, that's ugly. Is this needed for performance reasons?
================
Comment at: runtime/src/ompt-event-specific.h:25
-#define ompt_event_NEVER ompt_set_result_event_never_occurs
-#define ompt_event_UNIMPLEMENTED ompt_set_result_event_may_occur_no_callback
-#define ompt_event_MAY_CONVENIENT ompt_set_result_event_may_occur_callback_some
-#define ompt_event_MAY_ALWAYS ompt_set_result_event_may_occur_callback_always
+//#define ompt_event_NEVER ompt_set_never
+#define ompt_event_UNIMPLEMENTED ompt_set_never
----------------
Remove
================
Comment at: runtime/test/ompt/callback.h:70-92
+/*
#define print_frame(level)\
do {\
- printf("%" PRIu64 ": __builtin_frame_address(%d)=%p\n", ompt_get_thread_id(), level, __builtin_frame_address(level));\
+ unw_cursor_t cursor;\
+ unw_context_t uc;\
+ unw_word_t fp;\
+ unw_getcontext(&uc);\
----------------
Remove
================
Comment at: runtime/test/ompt/callback.h:115-130
+/*
+static void print_current_address()
+{
+ int real_level = 2;
+ void *array[real_level];
+ size_t size;
+ void *address;
----------------
Remove
================
Comment at: runtime/test/ompt/cancel/cancel_parallel.c:15-18
+ {} /* Empty block between "#pragma omp ..." and __asm__ statement as a workaround for icc bug */
+ __asm__("nop"); /* provide an instruction as jump target (compiler would insert an instruction if label is target of a jmp ) */
+ompt_label_1:
+ __asm__("nop");
----------------
There is a macro for this!
================
Comment at: runtime/test/ompt/loadtool/tool.c:1
+// RUN: true
+#include <stdio.h>
----------------
That's not really a test...
================
Comment at: runtime/test/ompt/synchronization/taskwait.c:34-37
+ // -CHECK: {{^}}[[THREAD_ID:[0-9]+]]: ompt_event_taskwait_begin: parallel_id={{[0-9]+}}, task_id={{[0-9]+}}, codeptr_ra=
+ // -CHECK: {{^}}[[THREAD_ID]]: ompt_event_wait_taskwait_begin: parallel_id={{[0-9]+}}, task_id={{[0-9]+}}, codeptr_ra=
+ // -CHECK: {{^}}[[THREAD_ID]]: ompt_event_wait_taskwait_end: parallel_id={{[0-9]+}}, task_id={{[0-9]+}}, codeptr_ra=
+ // -CHECK: {{^}}[[THREAD_ID]]: ompt_event_taskwait_end: parallel_id={{[0-9]+}}, task_id={{[0-9]+}}, codeptr_ra=
----------------
Why are these deactivated?
================
Comment at: runtime/test/ompt/tasks/task_types.c:111-112
+
+ // ___CHECK-DAG: {{^[0-9]+}}: ompt_event_task_create: parent_task_id={{[0-9]+}}, parent_task_frame.exit={{0x[0-f]+}}, parent_task_frame.reenter={{0x[0-f]+}}, new_task_id={{[0-9]+}}, codeptr_ra={{0x[0-f]+}}, task_type=ompt_task_explicit=4, has_dependences=no
+ // ___CHECK-DAG: {{^[0-9]+}}: id=7 task_type=ompt_task_explicit=4
+
----------------
Deactivated?
https://reviews.llvm.org/D38185
More information about the Openmp-commits
mailing list