diff mbox series

libgomp: Document 'GOMP_teams4' (was: GCN: Honor OpenMP 5.1 'num_teams' lower bound)

Message ID 87zfqh2wlc.fsf@euler.schwinge.ddns.net
State New
Headers show
Series libgomp: Document 'GOMP_teams4' (was: GCN: Honor OpenMP 5.1 'num_teams' lower bound) | expand

Commit Message

Thomas Schwinge July 16, 2024, 3:19 p.m. UTC 
Hi!

On 2024-07-15T17:01:46+0100, Andrew Stubbs <ams@baylibre.com> wrote:
> On 15/07/2024 16:36, Thomas Schwinge wrote:
>> On 2024-07-15T12:16:30+0100, Andrew Stubbs <ams@baylibre.com> wrote:
>>> On 15/07/2024 10:29, Thomas Schwinge wrote:
>>>> On 2021-11-12T18:58:04+0100, Jakub Jelinek via Gcc-patches <gcc-patches@gcc.gnu.org> wrote:
>>>>> And finally here is a third version, [...]
>>>>
>>>> ... which became commit 9fa72756d90e0d9edadf6e6f5f56476029925788
>>>> "libgomp, nvptx: Honor OpenMP 5.1 num_teams lower bound".
>>>>
>>>> Attached here is "GCN: Honor OpenMP 5.1 'num_teams' lower bound", which
>>>> are exactly the corresponding changes for GCN (see below Jakub's nvptx
>>>> changes for reference); OK to push?
>> 
>>> That's a lot of convoluted logic to drop in without a single comment!
>> 
>> Well, I'll pass that compliment over to Jakub ;-) -- my code changes just
>> intend to be a faithful "'s%nvptx%GCN'" of his code changes from back
>> then.
>> 
>>> The GCN bits look fine, and I assume you've probably thought about the
>>> logic here a lot, but I've no idea what you're trying to achieve, or why
>>> you're trying to achieve it (from the patch alone).
>>>
>>> Can we have some comments on motivation and goals, please?
>> 
>> Here's the original context:
>> 
>>    - <https://inbox.sourceware.org/20211111190313.GV2710@tucnak> "[PATCH] openmp: Honor OpenMP 5.1 num_teams lower bound"
>>    - <https://inbox.sourceware.org/20211112132023.GC2710@tucnak> "[PATCH] libgomp, nvptx: Honor OpenMP 5.1 num_teams lower bound"
>> 
>> Is that sufficient, and/or would you like to see some commentary to the
>> relevant libgomp generic/nvptx/GCN code added?
>
> Yes, sorry if it wasn't clear; I meant *code* comments.
>
> /* The team number is usually the same as the gcn_dim_pos(0), except 
> when num_teams(N) is .....   */
>
> The FIXME actually tells me something useful about one of the 
> conditional cases, but that's being removed here.
>
> Also, why are we returning "false" in other cases, and what effect does 
> that have? Is that for "spare" teams when we launch more than we need?

How about the attached "libgomp: Document 'GOMP_teams4'"?  Jakub, does
that accurately reflect the relevant facts?


Grüße
 Thomas
diff mbox series

Patch

From 149c2dc71bb44a9365ea3c360304f75cb9056084 Mon Sep 17 00:00:00 2001
From: Thomas Schwinge <tschwinge@baylibre.com>
Date: Tue, 16 Jul 2024 17:09:38 +0200
Subject: [PATCH] libgomp: Document 'GOMP_teams4'

For reference:

  - <https://inbox.sourceware.org/20211111190313.GV2710@tucnak> "[PATCH] openmp: Honor OpenMP 5.1 num_teams lower bound"
  - <https://inbox.sourceware.org/20211112132023.GC2710@tucnak> "[PATCH] libgomp, nvptx: Honor OpenMP 5.1 num_teams lower bound"

	libgomp/
	* config/gcn/target.c (GOMP_teams4): Document.
	* config/nvptx/target.c (GOMP_teams4): Likewise.
	* target.c (GOMP_teams4): Likewise.
---
 libgomp/config/gcn/target.c   | 8 ++++++++
 libgomp/config/nvptx/target.c | 8 ++++++++
 libgomp/target.c              | 9 +++++++++
 3 files changed, 25 insertions(+)

diff --git a/libgomp/config/gcn/target.c b/libgomp/config/gcn/target.c
index e57d2e5f93f..9cafea4e2cc 100644
--- a/libgomp/config/gcn/target.c
+++ b/libgomp/config/gcn/target.c
@@ -29,6 +29,14 @@ 
 
 extern volatile struct gomp_offload_icvs GOMP_ADDITIONAL_ICVS;
 
+/* Implement OpenMP 'teams' construct.
+
+   Initialize upon FIRST call.  Return whether this invocation is active.
+   Depending on whether NUM_TEAMS_LOWER asks for more teams than are provided
+   in hardware, we may need to loop multiple times; in that case make sure to
+   update the team-level variable used by 'omp_get_team_num', as we then can't
+   just use '__builtin_gcn_dim_pos (0)'.  */
+
 bool
 GOMP_teams4 (unsigned int num_teams_lower, unsigned int num_teams_upper,
 	     unsigned int thread_limit, bool first)
diff --git a/libgomp/config/nvptx/target.c b/libgomp/config/nvptx/target.c
index f14dcfc8ff6..8d4dc5f661a 100644
--- a/libgomp/config/nvptx/target.c
+++ b/libgomp/config/nvptx/target.c
@@ -31,6 +31,14 @@  extern int __gomp_team_num __attribute__((shared));
 extern volatile struct gomp_offload_icvs GOMP_ADDITIONAL_ICVS;
 volatile struct rev_offload *GOMP_REV_OFFLOAD_VAR;
 
+/* Implement OpenMP 'teams' construct.
+
+   Initialize upon FIRST call.  Return whether this invocation is active.
+   Depending on whether NUM_TEAMS_LOWER asks for more teams than are provided
+   in hardware, we may need to loop multiple times; in that case make sure to
+   update the team-level variable used by 'omp_get_team_num', as we then can't
+   just use '%ctaid.x'.  */
+
 bool
 GOMP_teams4 (unsigned int num_teams_lower, unsigned int num_teams_upper,
 	     unsigned int thread_limit, bool first)
diff --git a/libgomp/target.c b/libgomp/target.c
index e311e5a8302..47a18477b2a 100644
--- a/libgomp/target.c
+++ b/libgomp/target.c
@@ -4355,6 +4355,8 @@  gomp_target_task_fn (void *data)
   return false;
 }
 
+/* Implement OpenMP 'teams' construct, legacy entry point.  */
+
 void
 GOMP_teams (unsigned int num_teams, unsigned int thread_limit)
 {
@@ -4367,6 +4369,13 @@  GOMP_teams (unsigned int num_teams, unsigned int thread_limit)
   (void) num_teams;
 }
 
+/* Implement OpenMP 'teams' construct.
+
+   Initialize upon FIRST call.  Return whether this invocation is active.
+   Depending on whether NUM_TEAMS_LOW asks for more teams than are provided
+   in hardware, we may need to loop multiple times; in that case make sure to
+   update the team-level variable used by 'omp_get_team_num'.  */
+
 bool
 GOMP_teams4 (unsigned int num_teams_low, unsigned int num_teams_high,
 	     unsigned int thread_limit, bool first)
-- 
2.34.1