[Openmp-commits] [PATCH] D146642: [OpenMP] Implement task record and replay mechanism

[Jose Manuel Monsalve Diaz via Phabricator via Openmp-commits]

josemonsalve2 added a comment.

Adding some initial comments



================
Comment at: openmp/runtime/src/kmp.h:2485
+// Maximum number of TDGs
+#define NUM_TDG_LIMIT 100
+// Initial number of allocated nodes while recording
----------------
It may be better to expose these as env variables that can be configured by the user


================
Comment at: openmp/runtime/src/kmp.h:2497
+
+//Represents a TDG node
+typedef struct kmp_node_info {
----------------
This should probably use doxygen comments `///`


================
Comment at: openmp/runtime/src/kmp.h:2498-2506
+typedef struct kmp_node_info {
+  kmp_task_t *task; //Pointer to the actual task
+  kmp_int32 *successors; //Array of the succesors ids
+  kmp_int32 nsuccessors; //Number of succesors of the node
+  std::atomic<kmp_int32> npredecessors_counter; //Number of predessors on the fly
+  kmp_int32 npredecessors; //Total number of predecessors
+  kmp_int32 successors_size; // Number of allocated succesors ids
----------------
Remember to run clang-format on the changes through the git hook


================
Comment at: openmp/runtime/src/kmp.h:2513
+  kmp_int32 tdg_id; // Unique idenfifier of the TDG
+  kmp_taskgraph_flags_t tdg_flags; // Flags related to a tdg
+  kmp_int32 map_size; // Number of allocated TDG nodes
----------------
Be consistent with the TDG in upper case
```
// Flags related to a TDG
```


================
Comment at: openmp/runtime/src/kmp_taskdeps.cpp:224
+  if((source->dn.task && sink_task) && ((task_source->is_taskgraph && !task_sink->is_taskgraph) || (!task_source->is_taskgraph && task_sink->is_taskgraph))){
+    printf("Internal OpenMP error: task dependency detected between a task inside a taskgraph and a task outside, this is not supported \n");
+  }
----------------
I believe `KMP_ASSERT` instead of `printf` is better. Others, please advice.


================
Comment at: openmp/runtime/src/kmp_taskdeps.cpp:293
+    kmp_tdg_status tdg_status = KMP_TDG_NONE;
+    if (task){
+      kmp_taskdata_t *td = KMP_TASK_TO_TASKDATA(task);
----------------
This should be `if (task) {` with a leading space before the bracket. But `clang-format` can help with these


================
Comment at: openmp/runtime/src/kmp_taskdeps.h:98
+    // TODO: Not needed when taskifying
+    // printf("[OpenMP] ---- Task %d ends, checking successors ----\n",
+    // this_task->part_id);
----------------
Remove commented code that's not used.


================
Comment at: openmp/runtime/src/kmp_taskdeps.h:105
+      kmp_node_info_t *successor = &(task->tdg->record_map[successorNumber]);
+      // printf("  [OpenMP] Found one successor %d , deps : %d \n",
+      // successorNumber, successor->npredecessors_counter);
----------------
You may be able to use the macros in `kmp_debug.h` for these messages


================
Comment at: openmp/runtime/src/kmp_tasking.cpp:3470
 #endif
-            KMP_ATOMIC_DEC(unfinished_threads);
+                          KMP_ATOMIC_DEC(unfinished_threads);
         KA_TRACE(20, ("__kmp_execute_tasks_template: T#%d dec "
----------------
Not needed


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146642