Re: [PATCH]: libgompd add parallel handle functions

[Jakub Jelinek <jakub@redhat.com>]

On Mon, Jun 06, 2022 at 07:32:31PM +0200, Mohamed Atef wrote:
> So for both cases one should read the value of *team and if it's NULL, the
> function returns some error state (eg. ompd_rc_unavailable)

No, I think for team NULL it should simply push something different
to ompd_parallel_handle_t, something that would mean for uses of the handle
that it is not a normal explicit parallel, but the implicit parallel
and handle those cases differently.
E.g. if one has
int
main ()
{
  #pragma omp parallel
  sleep (1024);
}
then when you get the explicit parallel's handle, there is struct gomp_team
one can store and work with, but its enclosing parallel isn't non-existent,
it is an implicit parallel, only its enclosing parallel doesn't exist.

Unfortunately, it isn't that easy.  We sometimes do create struct gomp_team
even for the implicit parallel.
See libgomp/target.c (GOMP_target_ext) which is there for cases like
int
main ()
{
  #pragma omp target nowait
  something;
  something_else;
  #pragma omp taskwait
}
where we want the asynchronous target to be really asynchronous with
something_else; and need struct gomp_team for that.
But in the case of artificial struct gomp_team for this case
thr->ts.level will be 0 rather than > 0.

> > ompd_get_task_parallel_handle when you'll have struct gomp_task *
> > and want the struct gomp_team it is in.
> > I'm afraid the library doesn't track this, it doesn't need it for anything.
> > One possibility to resolve this is perhaps if all functions that
> > allocate ompd_task_handle_t can't know the corresponding struct gomp_thread
> > too, then you could store in the private structure or ompd_task_handle_t
> > both struct gomp_task * and struct gomp_thread *.
> >
> I will ask the guys to try this if it's impossible then we delay this
> function.

Perhaps the function can be added but could just error unconditionally
until some solution is found.

BTW, when looking at the patch, I found I've missed some things in the first
already committed patch.

+  #define gompd_init_access(t, m)  \                                                                                                                                                 
+    gompd_access_##t##_##m = (__UINT64_TYPE__) & (((struct t *) NULL)->m);                                                                                                           
This is UB, should be using offsetof (struct t, m) instead.

Also, using __UINT64_TYPE__ for those offsets or sizes seems to be very
excessive, on x86_64-linux the largest struct from looking at debug info
is struct gomp_team right now with 1344 bytes.
So for the time being, I think using __UINT16_TYPE__ for all those sizes and
offsets should be 4 times as compact.

And, it would be nice to initialize those at least when possible at compile
time, not in gompd_load, offsetof of a non-VLA type is a constant
expression, similarly sizeof, so making all those const and initialized
directly would mean they don't waste a writable section (so all processes
can share those).  Probably it would be nice to stick them at least for ELF
into a names section so that they'd be together and not needing to be ever
touched unless OMPD is enabled.
Of course, I don't rule out the possibility of some values needing to be
initialized at runtime, they'd simply just not be const like the rest.

	Jakub