[OpenMP] Fix Taskgraph bugs #136837

jpinot · 2025-04-23T09:59:29Z

This PR includes a set of fixes related to taskgraph support in OpenMP:

Initialize taskgraph-related fields in task allocation, to fix uninitialized access.
Fix out-of-bounds access in __kmpc_omp_task_with_deps.
Fix missing copy of successors array during reallocation in taskgraph.

Fix a potential use of uninitialized is_taskgraph and tdg fields when a task is created outside of a taskgraph construct. These fields were previously left uninitialized, which could lead to undefined behavior when later accessed.

… incorrect task ID Correct use of task ID field when accessing the taskgraph’s record_map. The previous logic mistakenly used td_task_id instead of td_tdg_task_id, potentially causing out-of-bounds memory access when td_task_id exceeded the map_size. td_task_id and td_tdg_task_id was introduced in llvm#130660

shiltian · 2025-04-23T18:54:20Z

openmp/runtime/src/kmp_taskdeps.cpp

+        kmp_uint old_size = source_info->successors_size;
+        kmp_uint new_size = source_info->successors_size * 2;
        kmp_int32 *old_succ_ids = source_info->successors;
-        kmp_int32 *new_succ_ids = (kmp_int32 *)__kmp_allocate(
-            source_info->successors_size * sizeof(kmp_int32));
+        kmp_int32 *new_succ_ids =
+            (kmp_int32 *)__kmp_allocate(new_size * sizeof(kmp_int32));
+        KMP_MEMCPY(new_succ_ids, old_succ_ids, old_size * sizeof(kmp_int32));
        source_info->successors = new_succ_ids;
+        source_info->successors_size = new_size;


You don't actually need the new_size and related code. Only old_size is needed and all the other code can stay unchanged. The copy only uses old_size anyway.

shiltian · 2025-04-23T18:54:55Z

openmp/runtime/src/kmp_tasking.cpp

 }

-// __kmp_start_record: launch the execution of a previous
+// __kmp_exec_tdg: launch the execution of a previous


A NFC change.

I Updated to commit message to [NFC][OpenMP] Fix task record/replay comments,
I noticed a few different formatting styles in the commit history—do you have a preferred convention?

That should go in a separate PR. LLVM only allows squash commits when merging a PR.

As for coding style, we don't really have a strict standard for libomp. I'd recommend following the older code style. By "older," I mean the code from around 2017, not the stuff added in the last year or two.

Got it 😊 Thanks for the heads-up!

shiltian · 2025-04-23T18:55:32Z

openmp/runtime/src/kmp_tasking.cpp


 #if OMPX_TASKGRAPH
 // __kmp_find_tdg: identify a TDG through its ID
-// gtid:   Global Thread ID


Make an NFC change to fix the code comments. Include the other one.

same as above

rneveu · 2025-04-23T21:20:42Z

As these changes correct issues with dynamic arrays in the task graph structure, should we also add tests regarding the size and width of the task graph?

jpinot · 2025-04-24T10:32:55Z

As these changes correct issues with dynamic arrays in the task graph structure, should we also add tests regarding the size and width of the task graph?

I added a new test that focuses on TDG with dependencies and multiple successors. While there is already a test for TDG with dependencies (omp_record_replay_deps.cpp), it doesn’t exercise the expansion of the successors array directly.

Let me know you thoughts on whether keeping this as a new targeted test, or if it would be better to update the existing test to incorporate these scenarios, even if it adds some complexity.

shiltian

The fix looks reasonable to me. LGTM as long as @rneveu is happy with the test changes.

shiltian · 2025-04-24T13:29:39Z

openmp/runtime/test/tasking/omp_record_replay_deps_multi_succ.cpp

+
+#define TASKS_SIZE 12
+
+typedef struct ident {


It doesn't need to have a definition. A forward declaration would be sufficient.

shiltian · 2025-04-24T13:32:54Z

openmp/runtime/test/tasking/omp_record_replay_deps_multi_succ.cpp

+  void *dummy;
+} ident_t;
+
+#ifdef __cplusplus


No need to use #ifdef __cplusplus since it is already %libomp-cxx-compile-and-run

shiltian · 2025-04-24T13:33:17Z

openmp/runtime/test/tasking/omp_record_replay_deps_multi_succ.cpp

+#include <cassert>
+#include <vector>
+
+#define TASKS_SIZE 12


I'd use constexpr const.

Ensure proper resizing and copying of the successors array when its capacity is exceeded. The previous implementation allocated a new array but did not copy the existing elements. This led to loss of successor data and incorrect task dependency tracking.

rneveu · 2025-04-24T16:31:07Z

LGTM

…lization (llvm#136837) This commit resolves multiple issues in the OpenMP taskgraph implementation: - Fix a potential use of uninitialized is_taskgraph and tdg fields when a task is created outside of a taskgraph construct. - Fix use of task ID field when accessing the taskgraph’s record_map. - Fix resizing and copying of the successors array when its capacity is exceeded. Fixes memory management flaws, invalid memory accesses, and uninitialized data risks in taskgraph operations.

jpinot changed the title ~~[Draft][OpenMP] Openmp taskgraph bug fix~~ [Draft][OpenMP] Fix Taskgraph bugs Apr 23, 2025

jpinot added 2 commits April 23, 2025 12:08

jpinot force-pushed the openmp_taskgraph_bug_fix branch 2 times, most recently from 648e961 to 126257d Compare April 23, 2025 10:21

jpinot changed the title ~~[Draft][OpenMP] Fix Taskgraph bugs~~ [OpenMP] Fix Taskgraph bugs Apr 23, 2025

jpinot marked this pull request as ready for review April 23, 2025 11:25

llvmbot added the openmp:libomp OpenMP host runtime label Apr 23, 2025

jpinot requested review from jdoerfert, josemonsalve2 and shiltian April 23, 2025 11:25

shiltian reviewed Apr 23, 2025

View reviewed changes

jpinot force-pushed the openmp_taskgraph_bug_fix branch from 126257d to 3d86d1d Compare April 24, 2025 10:18

jpinot requested a review from shiltian April 24, 2025 10:33

jpinot force-pushed the openmp_taskgraph_bug_fix branch from 3d86d1d to 2c7db5f Compare April 24, 2025 13:24

shiltian approved these changes Apr 24, 2025

View reviewed changes

jpinot force-pushed the openmp_taskgraph_bug_fix branch from 2c7db5f to b13090d Compare April 24, 2025 13:44

jpinot merged commit ada4ad9 into llvm:main Apr 25, 2025
9 checks passed

jpinot deleted the openmp_taskgraph_bug_fix branch April 28, 2025 06:46

[OpenMP] Fix Taskgraph bugs #136837

[OpenMP] Fix Taskgraph bugs #136837

Uh oh!

Conversation

jpinot commented Apr 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rneveu commented Apr 23, 2025

Uh oh!

jpinot commented Apr 24, 2025

Uh oh!

shiltian left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rneveu commented Apr 24, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jpinot commented Apr 23, 2025 •

edited

Loading