Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Phabricator shutdown timeline

Differential D137748 Diff 478338 openmp/runtime/test/tasking/bug_nested_proxy_task.c

Changeset View

Standalone View

View Options

openmp/runtime/test/tasking/bug_nested_proxy_task.c

Show All 14 Lines
With task dependencies one can generate proxy tasks from an explicit task With task dependencies one can generate proxy tasks from an explicit task
being executed by a serial task team. The OpenMP runtime library didn't being executed by a serial task team. The OpenMP runtime library didn't
expect that and tries to free the explicit task that is the parent of the expect that and tries to free the explicit task that is the parent of the
proxy task still working in background. It therefore has incomplete children proxy task still working in background. It therefore has incomplete children
which triggers a debugging assertion. which triggers a debugging assertion.
*/ */
// Compiler-generated code (emulation) // Compiler-generated code (emulation)
#ifdef _WIN64
typedef long long kmp_intptr_t;
#else
typedef long kmp_intptr_t; typedef long kmp_intptr_t;
#endif
typedef int kmp_int32; typedef int kmp_int32;
typedef unsigned char kmp_uint8;
jlpeytonUnsubmitted
Not Done
ReplyInline Actions

Can we #include stdint.h above and then use

typedef intptr_t kmp_intptr_t;
typedef int32_t kmp_int32;
typedef uint8_t kmp_uint8;
jlpeyton: Can we `#include stdint.h` above and then use ``` typedef intptr_t kmp_intptr_t; typedef…
mstorsjoAuthorUnsubmitted
Done
ReplyInline Actions

That works for me too. But can we do it in two separate patches - first this, and then changing to using stdint.h? In case the stdint.h change breaks something unexpected in a weird environment and the change needs to be reverted, we'd still have the tests fixed on Win64.

mstorsjo: That works for me too. But can we do it in two separate patches - first this, and then changing…
typedef char bool; typedef char bool;
// These structs need to match the implementation within libomp, in kmp.h.
typedef struct ident { typedef struct ident {
kmp_int32 reserved_1; /**< might be used in Fortran; see above */ kmp_int32 reserved_1; /**< might be used in Fortran; see above */
kmp_int32 flags; /**< also f.flags; KMP_IDENT_xxx flags; KMP_IDENT_KMPC identifies this union member */ kmp_int32 flags; /**< also f.flags; KMP_IDENT_xxx flags; KMP_IDENT_KMPC identifies this union member */
kmp_int32 reserved_2; /**< not really used in Fortran any more; see above */ kmp_int32 reserved_2; /**< not really used in Fortran any more; see above */
#if USE_ITT_BUILD #if USE_ITT_BUILD
/* but currently used for storing region-specific ITT */ /* but currently used for storing region-specific ITT */
/* contextual information. */ /* contextual information. */
#endif /* USE_ITT_BUILD */ #endif /* USE_ITT_BUILD */
kmp_int32 reserved_3; /**< source[4] in Fortran, do not use for C++ */ kmp_int32 reserved_3; /**< source[4] in Fortran, do not use for C++ */
char const *psource; /**< String describing the source location. char const *psource; /**< String describing the source location.
The string is composed of semi-colon separated fields which describe the source file, The string is composed of semi-colon separated fields which describe the source file,
the function and a pair of line numbers that delimit the construct. the function and a pair of line numbers that delimit the construct.
*/ */
} ident_t; } ident_t;
typedef struct kmp_depend_info { typedef struct kmp_depend_info {
kmp_intptr_t base_addr; kmp_intptr_t base_addr;
size_t len; size_t len;
struct { union {
bool in:1; kmp_uint8 flag; // flag as an unsigned char
bool out:1; struct { // flag as a set of 8 bits
unsigned in : 1;
natglaUnsubmitted
Not Done
ReplyInline Actions

sorry for being late here. if the bit field is defined as unsigned, the set of bit fields will take at least "unsigned" space (4 bytes). Judging by union with flag: do you want to define all these as kmp_uint8? (same below)

natgla: sorry for being late here. if the bit field is defined as unsigned, the set of bit fields will…
mstorsjoAuthorUnsubmitted
Done
ReplyInline Actions

No, here, I want exactly this - and your comment is even more reason for it.

Here, we’re redefining an libomp internal struct, and exactly for rains like this, we really should have an exactly matching definition to the one in kmp.h in the source tree.

You’re right that we probably should change it like you’re suggesting - but then we should change kmp.h too, in sync with this.

mstorsjo: No, here, I want exactly this - and your comment is even more reason for it. Here, we’re…
natglaUnsubmitted
Not Done
ReplyInline Actions

would you mind adding a comment on kmp_depend_info explaining that it's exact copy of kmp_depend_info from kmp.h? (or if there are other structs here, more general comment?)

natgla: would you mind adding a comment on kmp_depend_info explaining that it's exact copy of…
mstorsjoAuthorUnsubmitted
Done
ReplyInline Actions

Sure, I can do that.

In practice, I have no idea how this is meant to be used in practice - is this something that regular users of OpenMP are supposed to do, or is it just a intrusive test? But in any case, this brings the testsuite to passing in more cases.

mstorsjo: Sure, I can do that. In practice, I have no idea how this is meant to be used in practice - is…
natglaUnsubmitted
Not Done
ReplyInline Actions

looks like a unit test for runtime to me. It's emulating what compiler would generate (see comment at the top of the test)

natgla: looks like a unit test for runtime to me. It's emulating what compiler would generate (see…
unsigned out : 1;
unsigned mtx : 1;
unsigned set : 1;
unsigned unused : 3;
unsigned all : 1;
} flags; } flags;
};
} kmp_depend_info_t; } kmp_depend_info_t;
struct kmp_task; struct kmp_task;
typedef kmp_int32 (* kmp_routine_entry_t)( kmp_int32, struct kmp_task * ); typedef kmp_int32 (* kmp_routine_entry_t)( kmp_int32, struct kmp_task * );
typedef struct kmp_task { /* GEH: Shouldn't this be aligned somehow? */ typedef struct kmp_task { /* GEH: Shouldn't this be aligned somehow? */
void * shareds; /**< pointer to block of pointers to shared vars */ void * shareds; /**< pointer to block of pointers to shared vars */
kmp_routine_entry_t routine; /**< pointer to routine to call for executing task */ kmp_routine_entry_t routine; /**< pointer to routine to call for executing task */
▲ Show 20 Lines • Show All 42 Lines • ▼ Show 20 Lines
{ {
/* /*
* Corresponds to: * Corresponds to:
#pragma omp target nowait depend(out: dep) #pragma omp target nowait depend(out: dep)
{ {
my_sleep( 0.1 ); my_sleep( 0.1 );
} }
*/ */
kmp_depend_info_t dep_info; kmp_depend_info_t dep_info = { 0 };
dep_info.base_addr = (long) &dep; dep_info.base_addr = (kmp_intptr_t) &dep;
dep_info.len = sizeof(int); dep_info.len = sizeof(int);
// out = inout per spec and runtime expects this // out = inout per spec and runtime expects this
dep_info.flags.in = 1; dep_info.flags.in = 1;
dep_info.flags.out = 1; dep_info.flags.out = 1;
kmp_int32 gtid = __kmpc_global_thread_num(NULL); kmp_int32 gtid = __kmpc_global_thread_num(NULL);
kmp_task_t *proxy_task = __kmpc_omp_task_alloc(NULL,gtid,17,sizeof(kmp_task_t),0,&task_entry); kmp_task_t *proxy_task = __kmpc_omp_task_alloc(NULL,gtid,17,sizeof(kmp_task_t),0,&task_entry);
__kmpc_omp_task_with_deps(NULL,gtid,proxy_task,1,&dep_info,0,NULL); __kmpc_omp_task_with_deps(NULL,gtid,proxy_task,1,&dep_info,0,NULL);
Show All 18 Lines