[OpenMP] OpenMP ThreadSet clause - basic runtime #144409

Ritanya-B-Bharadwaj · 2025-06-16T18:31:37Z

Initial runtime support for threadset clause in task and taskloop directives [Section 14.8 in in OpenMP 6.0 spec]

Frontend PR- #135807

github-actions · 2025-06-16T18:35:15Z

✅ With the latest revision this PR passed the C/C++ code formatter.

alexey-bataev · 2025-06-17T14:27:50Z

openmp/runtime/src/kmp.h

Put free_agent_eligible before reserved

It looks like you need to use reversed order for big endian here

#if OMPX_TASKGRAPH unsigned reserved31 : 4; unsigned onced : 1; #else unsigned reserved31 : 5; #endif unsigned hidden_helper : 1; unsigned target : 1; unsigned native : 1; unsigned freed : 1; unsigned complete : 1; unsigned executing : 1; unsigned started : 1; unsigned team_serial : 1; unsigned tasking_ser : 1; unsigned task_serial : 1; unsigned tasktype : 1; unsigned reserved : 8; unsigned free_agent_eligible : 1; unsigned detachable : 1; unsigned priority_specified : 1; unsigned proxy : 1; unsigned destructors_thunk : 1; unsigned merged_if0 : 1; unsigned final : 1; unsigned tiedness : 1;

jpeyton52 · 2025-06-17T15:54:45Z

openmp/runtime/src/kmp.h

+  unsigned reserved : 7; /* reserved for compiler use */
+  unsigned free_agent_eligible : 1; /* set if task can be executed by a


This needs to be reversed as well.

Shouldn't it be the reverse of the above?

If you take one bit out of a reserved bits, you need to make it before the remaining bits.

If you take one bit out of a reserved bits, you need to make it before the remaining bits.

If you look at the other bits, they are consumed in reverse order for this endianess. I'm still confused by inverting the bit order and not the byte order for endianess, but this is how they seem to be implemented.

oh, that reminds me of the big endian thingy that IBM folks fixed in the past.

/* Compiler flags */ /* Total compiler flags must be 16 bits */ unsigned tiedness : 1; /* 0: task is either tied (1) or untied (0) */ unsigned final : 1; /* 1: task is final(1) so execute immediately */ unsigned merged_if0 : 1; /* 2: no __kmpc_task_{begin/complete}_if0 calls in if0 code path */ unsigned destructors_thunk : 1; /* 3: set if the compiler creates a thunk to invoke destructors from the runtime */ unsigned proxy : 1; /* 4:task is a proxy task (it will be executed outside the context of the RTL) */ unsigned priority_specified : 1; /* 5: set if the compiler provides priority setting for the task */ unsigned detachable : 1; /* 6: 1 == can detach */ unsigned free_agent_eligible : 1; /* 7: set if task can be executed by a free-agent thread */ unsigned reserved : 8; /* reserved for compiler use */ /* Library flags */ /* Total library flags must be 16 bits */ unsigned tasktype : 1; /* task is either explicit(1) or implicit (0) */ unsigned task_serial : 1; // task is executed immediately (1) or deferred (0) unsigned tasking_ser : 1; // all tasks in team are either executed immediately // (1) or may be deferred (0) unsigned team_serial : 1; // entire team is serial (1) [1 thread] or parallel // (0) [>= 2 threads] /* If either team_serial or tasking_ser is set, task team may be NULL */ /* Task State Flags: */ unsigned started : 1; /* 1==started, 0==not started */ unsigned executing : 1; /* 1==executing, 0==not executing */ unsigned complete : 1; /* 1==complete, 0==not complete */ unsigned freed : 1; /* 1==freed, 0==allocated */ unsigned native : 1; /* 1==gcc-compiled task, 0==intel */ unsigned target : 1; unsigned hidden_helper : 1; /* 1 == hidden helper task */ #if OMPX_TASKGRAPH unsigned onced : 1; /* 1==ran once already, 0==never ran, record & replay purposes */ unsigned reserved31 : 4; /* reserved for library use */ #else unsigned reserved31 : 5; /* reserved for library use */ #endif```

shiltian

I don't see any meaningful changes out of this PR at this moment. This should definitely not be merged into the repo given its current shape.

jprotze · 2025-06-17T16:55:20Z

I don't see any meaningful changes out of this PR at this moment. This should definitely not be merged into the repo given its current shape.

This is a compliant stub implementation of the runtime to complement the codegen. There are other openmp features with a runtime stub implementation at a similar QoI level on runtime side (taskwait nowait, target nowait with depend, ...)

shiltian · 2025-06-17T17:05:56Z

I'd assume the runtime support is orthogonal to the front end support. For the runtime support, this is far from what is needed. Just to add one bit w/o anything else is not sufficient. Even if the front end emits code that uses the bit, it will be fine because it is reserved anyway. On the other hand, to add runtime support doesn't require front end support as well.

alexey-bataev · 2025-06-17T17:08:26Z

I'd assume the runtime support is orthogonal to the front end support. For the runtime support, this is far from what is needed. Just to add one bit w/o anything else is not sufficient. Even if the front end emits code that uses the bit, it will be fine because it is reserved anyway. On the other hand, to add runtime support doesn't require front end support as well.

Not quite. If the compiler claims some bits used for some particular purposes, we cannot keep this bit "reserved" in the runtime, it maybe a potential source of bugs in future

shiltian · 2025-06-17T17:14:02Z

I'm not saying we are gonna have that for a long run. Just speaking from the series of patch's perspective. Even if we can't, we can always merge runtime support first, and then do the front end work "later".

jprotze · 2025-06-17T17:46:29Z

The whole bitset got messed up. It used to be 16 bits for compiler use, 16 bits for runtime use:

/* Compiler flags */ /* Total compiler flags must be 16 bits */
  unsigned tiedness : 1; /* task is either tied (1) or untied (0) */
  unsigned final : 1; /* task is final(1) so execute immediately */
  unsigned merged_if0 : 1; /* no __kmpc_task_{begin/complete}_if0 calls in if0
                              code path */
  unsigned destructors_thunk : 1; /* set if the compiler creates a thunk to
                                     invoke destructors from the runtime */
  unsigned proxy : 1; /* task is a proxy task (it will be executed outside the
                         context of the RTL) */
  unsigned priority_specified : 1; /* set if the compiler provides priority
                                      setting for the task */
  unsigned detachable : 1; /* 1 == can detach */
  unsigned reserved : 9; /* reserved for compiler use */

Then someone added hidden_helper to the compiler flags. Is this bit ever set by the compiler, or should it be a runtime flag?

The new flag should definitely be added to the compiler flags partition. From my perspective it should replace the hidden_helper flag and the hidden_helper flag should move to the runtime partition.

jprotze

After reordering the bits, the PR lgtm.

jprotze · 2025-06-17T17:58:12Z

openmp/runtime/src/kmp.h

#if OMPX_TASKGRAPH unsigned reserved31 : 4; unsigned onced : 1; #else unsigned reserved31 : 5; #endif unsigned hidden_helper : 1; unsigned target : 1; unsigned native : 1; unsigned freed : 1; unsigned complete : 1; unsigned executing : 1; unsigned started : 1; unsigned team_serial : 1; unsigned tasking_ser : 1; unsigned task_serial : 1; unsigned tasktype : 1; unsigned reserved : 8; unsigned free_agent_eligible : 1; unsigned detachable : 1; unsigned priority_specified : 1; unsigned proxy : 1; unsigned destructors_thunk : 1; unsigned merged_if0 : 1; unsigned final : 1; unsigned tiedness : 1;

jprotze · 2025-06-17T18:00:40Z

openmp/runtime/src/kmp.h

+  unsigned reserved : 7; /* reserved for compiler use */
+  unsigned free_agent_eligible : 1; /* set if task can be executed by a


/* Compiler flags */ /* Total compiler flags must be 16 bits */ unsigned tiedness : 1; /* 0: task is either tied (1) or untied (0) */ unsigned final : 1; /* 1: task is final(1) so execute immediately */ unsigned merged_if0 : 1; /* 2: no __kmpc_task_{begin/complete}_if0 calls in if0 code path */ unsigned destructors_thunk : 1; /* 3: set if the compiler creates a thunk to invoke destructors from the runtime */ unsigned proxy : 1; /* 4:task is a proxy task (it will be executed outside the context of the RTL) */ unsigned priority_specified : 1; /* 5: set if the compiler provides priority setting for the task */ unsigned detachable : 1; /* 6: 1 == can detach */ unsigned free_agent_eligible : 1; /* 7: set if task can be executed by a free-agent thread */ unsigned reserved : 8; /* reserved for compiler use */ /* Library flags */ /* Total library flags must be 16 bits */ unsigned tasktype : 1; /* task is either explicit(1) or implicit (0) */ unsigned task_serial : 1; // task is executed immediately (1) or deferred (0) unsigned tasking_ser : 1; // all tasks in team are either executed immediately // (1) or may be deferred (0) unsigned team_serial : 1; // entire team is serial (1) [1 thread] or parallel // (0) [>= 2 threads] /* If either team_serial or tasking_ser is set, task team may be NULL */ /* Task State Flags: */ unsigned started : 1; /* 1==started, 0==not started */ unsigned executing : 1; /* 1==executing, 0==not executing */ unsigned complete : 1; /* 1==complete, 0==not complete */ unsigned freed : 1; /* 1==freed, 0==allocated */ unsigned native : 1; /* 1==gcc-compiled task, 0==intel */ unsigned target : 1; unsigned hidden_helper : 1; /* 1 == hidden helper task */ #if OMPX_TASKGRAPH unsigned onced : 1; /* 1==ran once already, 0==never ran, record & replay purposes */ unsigned reserved31 : 4; /* reserved for library use */ #else unsigned reserved31 : 5; /* reserved for library use */ #endif```

Ritanya-B-Bharadwaj · 2025-06-29T14:28:23Z

@jprotze I have modified based on your suggestions.

jprotze

Lgtm

jpeyton52

LGTM

We need the stub implementation for consistency between compiler and runtime bits

Ritanya-B-Bharadwaj · 2025-07-08T12:19:32Z

@jprotze the frontend PR for this feature is still open - #135807
Can you please review it?

shiltian · 2025-07-08T16:14:09Z

We need the stub implementation for consistency between compiler and runtime bits

Just for the record, correct me if I'm wrong, but I don't think this is necessary for consistency between the compiler and runtime, because the bit isn't set by the compiler. I'm not comfortable merging a non-NFC change without any test case. On the other hand, even if it is set by the front end, the runtime changes can always go before the compiler change with a proper test case. The test case itself can simply manually construct the necessary function call that the compiler is expected to emit.

jprotze · 2025-07-08T17:43:10Z

We need the stub implementation for consistency between compiler and runtime bits

Just for the record, correct me if I'm wrong, but I don't think this is necessary for consistency between the compiler and runtime, because the bit isn't set by the compiler.

You are wrong. Or, what do you mean with "because the bit isn't set by the compiler"? Sure, the compiler doesn't set the bit literally. The bit is set in compiler-generated code injected into the application code (https://github.com/llvm/llvm-project/pull/135807/files#diff-ab54595e29e58ad74c94b0ecaf4d2f32d626345db3ae734aa7cf7d5e404ed499R3716). The compiler-generated code sets the bit, if threadset(omp_pool) is present on the task construct. The bit will be visible in the flags argument of __kmpc_omp_task_alloc, which is a call that only compiler-generated code should call (thus _kmpc prefix).

I'm happy to add an OMPT-test as soon as the frontend patch has landed.

shiltian · 2025-07-08T17:48:00Z

Then check this part of my previous comment:

even if it is set by the front end, the runtime changes can always go before the compiler change with a proper test case. The test case itself can simply manually construct the necessary function call that the compiler is expected to emit.

We already have a bunch of tests in libomp that "emulates" code generated by the compiler.

TerryLWilmarth · 2025-07-08T18:43:13Z

Then check this part of my previous comment:

even if it is set by the front end, the runtime changes can always go before the compiler change with a proper test case. The test case itself can simply manually construct the necessary function call that the compiler is expected to emit.

We already have a bunch of tests in libomp that "emulates" code generated by the compiler.

That's right -- you could write a test that calls __kmp_omp_task_alloc directly with the new bit set, as if it were done by the compiler. It will be a bit boring right now, since __kmp_omp_task_alloc doesn't do anything with it, but sufficient to actually do that inside a parallel region and make sure that it at least runs the task and doesn't break anything.

That's the only interaction between the compiler and runtime that we see happening in our implementation right now. We're working on the execution model changes and free agent threads now, but likely you will see the execution model changes first (if I can successfully keep it separate in any reasonable way). I'm all for having the front end in place earlier than the runtime in this case, because writing tests that emulate the compiler codegen to thoroughly test such things is tedious. That said, if the front end isn't ready, we can provide a more thorough test with the free agent threads contribution.

jprotze · 2025-07-08T20:13:22Z

We already have a bunch of tests in libomp that "emulates" code generated by the compiler.

These tests emulate code generation, when CodeGen lacks behind while a new feature is implemented in the runtime and should be tested for functionality. Here we have the opposite situation: runtime implementation lacks behind CodeGen. We still want to have the usage of the bit documented on both sides of the interface.

llvmbot added the openmp:libomp OpenMP host runtime label Jun 16, 2025

Ritanya-B-Bharadwaj mentioned this pull request Jun 16, 2025

[clang][OpenMP] New OpenMP 6.0 threadset clause #135807

Open

Ritanya-B-Bharadwaj requested a review from alexey-bataev June 16, 2025 18:32

Ritanya-B-Bharadwaj force-pushed the threadset-runtime branch from 54f3860 to ca3f29a Compare June 16, 2025 18:39

alexey-bataev reviewed Jun 17, 2025

View reviewed changes

[clang][OpenMP] OpenMP ThreadSet clause - basic runtime

4553bd2

Ritanya-B-Bharadwaj force-pushed the threadset-runtime branch from ca3f29a to 4553bd2 Compare June 17, 2025 15:43

alexey-bataev requested review from jdoerfert and shiltian June 17, 2025 15:44

jpeyton52 reviewed Jun 17, 2025

View reviewed changes

shiltian changed the title ~~[clang][OpenMP] OpenMP ThreadSet clause - basic runtime~~ [OpenMP] OpenMP ThreadSet clause - basic runtime Jun 17, 2025

shiltian previously requested changes Jun 17, 2025

View reviewed changes

jprotze reviewed Jun 17, 2025

View reviewed changes

Reordering the flags in kmp.h based on review comments

b40dd81

Ritanya-B-Bharadwaj requested review from alexey-bataev, jpeyton52 and jprotze July 3, 2025 08:00

jprotze approved these changes Jul 3, 2025

View reviewed changes

jpeyton52 approved these changes Jul 3, 2025

View reviewed changes

jprotze merged commit 0f391d6 into llvm:main Jul 8, 2025
7 checks passed

		unsigned reserved : 7; /* reserved for compiler use */
		unsigned free_agent_eligible : 1; /* set if task can be executed by a

[OpenMP] OpenMP ThreadSet clause - basic runtime #144409

[OpenMP] OpenMP ThreadSet clause - basic runtime #144409

Uh oh!

Conversation

Ritanya-B-Bharadwaj commented Jun 16, 2025

Uh oh!

github-actions bot commented Jun 16, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shiltian left a comment

Choose a reason for hiding this comment

Uh oh!

jprotze commented Jun 17, 2025

Uh oh!

shiltian commented Jun 17, 2025

Uh oh!

alexey-bataev commented Jun 17, 2025

Uh oh!

shiltian commented Jun 17, 2025

Uh oh!

jprotze commented Jun 17, 2025

Uh oh!

jprotze 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!

Ritanya-B-Bharadwaj commented Jun 29, 2025

Uh oh!

jprotze left a comment

Choose a reason for hiding this comment

Uh oh!

jpeyton52 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Ritanya-B-Bharadwaj commented Jul 8, 2025

Uh oh!

shiltian commented Jul 8, 2025

Uh oh!

jprotze commented Jul 8, 2025

Uh oh!

shiltian commented Jul 8, 2025

Uh oh!

TerryLWilmarth commented Jul 8, 2025

Uh oh!

jprotze commented Jul 8, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

github-actions bot commented Jun 16, 2025 •

edited

Loading