Unit tests: unknown.

clang-tidy: unknown.

clang-format: unknown.

Build artifacts: diff.json, console-log.txt

the task create and task issue step are
conceptually not separated anymore as it is

I don't think this can work reliably. Because not all C++ objects can be mem-copied.
E.g. an object can keep its own address or reference, and mem-copy will make it broken.
This could be fixed by generating (optional) thunk routine which would create all needed objects
in the library-allocated space, and similar routine which would destroy all the created objects.

In D71989#1819219, @AndreyChurbanov wrote:

the task create and task issue step are
conceptually not separated anymore as it is

I don't think this can work reliably. Because not all C++ objects can be mem-copied.
E.g. an object can keep its own address or reference, and mem-copy will make it broken.
This could be fixed by generating (optional) thunk routine which would create all needed objects
in the library-allocated space, and similar routine which would destroy all the created objects.

I agree. memcpy will only work for certain types (incl. all PoD types). We need to extend this later to pass the copy constructor function pointers and locations of the original object.
We should keep kmp_uint32 sizeof_shared_and_private_vars, void *shared_and_private_vars for the simple cases and add something like:

/// Copy wrapper takes the address of an object and the address the copy is going to be initialized in and returns the address right after the new object. 
typedef void*(*copy_wrapper)(void * /* src_addr */, void * /* trg_addr */);
 
__kmpc_task( ..., kmp_uint32 sizeof_copy_infos, copy_wrapper * copy_wrapper_list, void * copied_obj_list, ...)

and we then call the copy constructors like this:

void *local_addr = ...;
for (kmp_uint32 u = 0; u < sizeof_copy_infos; ++u)
  local_addr = copy_wrapper_list[u](copied_obj_list[u], local_addr);

@rogfer01 @AndreyChurbanov What do you think? (I can do this in this patch or later)

and we then call the copy constructors like this:

void *local_addr = ...;
for (kmp_uint32 u = 0; u < sizeof_copy_infos; ++u)
  local_addr = copy_wrapper_list[u](copied_obj_list[u], local_addr);

Just to confirm: that local_addr would be somehow linked to the task, I imagine it'd be initialized to something like task->shareds + offset_to_firstprivates, wouldn't it?

Also, perhaps you already considered if it makes sense to just have a copy function generated by the front-end rather than one of for each "firstprivatized variable that can't just be memcpy'ed"?

Also I'm curious why are you moving away (perhaps I did get this wrong!) from the current model of

1. kmpc_omp_task_alloc
2. capture environment
3. queue the task kmpc_omp_taskor (if the task is if(0)) do an "immediate" execution but use the environment captured in 2

to something that looks like

1. (partially?) capture the environment. I think I didn't understand what void *shared_and_private_vars will do here. Is this something that the front-end precaptured for us?
2. copy the environment you obtained from shared_and_private_vars to a task-local storage and then queue the task or (if the task is if(0)) do an immediate execution using the environment you got from the argument to kmpc_task

I guess there is a benefit in the new approach. So far I read this as improving the if(0) case (but I may be missing something here).

Thanks!

In D71989#1820289, @rogfer01 wrote:

and we then call the copy constructors like this:

void *local_addr = ...;
for (kmp_uint32 u = 0; u < sizeof_copy_infos; ++u)
  local_addr = copy_wrapper_list[u](copied_obj_list[u], local_addr);

Just to confirm: that local_addr would be somehow linked to the task, I imagine it'd be initialized to something like task->shareds + offset_to_firstprivates, wouldn't it?

Yes. It is some location in which the task local variables live.

Also, perhaps you already considered if it makes sense to just have a copy function generated by the front-end rather than one of for each "firstprivatized variable that can't just be memcpy'ed"?

Having a copy function per type allows us to reuse it, otherwise we have one copy function per static task location (at worst). Either works for me I think.

Also I'm curious why are you moving away (perhaps I did get this wrong!) from the current model of

1. kmpc_omp_task_alloc
2. capture environment
3. queue the task kmpc_omp_taskor (if the task is if(0)) do an "immediate" execution but use the environment captured in 2

to something that looks like

1. (partially?) capture the environment. I think I didn't understand what void *shared_and_private_vars will do here. Is this something that the front-end precaptured for us?

The idea is that we can allocate variables (for which we do not invoke a copy constructor) in a smart way so that we can easily copy them if we need to or not copy them at all if we don't. This interface is a first version though roughly designed like the tregion interface. Any suggestions are welcome!

2. copy the environment you obtained from shared_and_private_vars to a task-local storage and then queue the task or (if the task is if(0)) do an immediate execution using the environment you got from the argument to kmpc_task

Optimally, you would only copy if you need to.

I guess there is a benefit in the new approach. So far I read this as improving the if(0) case (but I may be missing something here).

There are multiple reasons but the main one for me is for sure the ability to use callback metadata to link the values passed at the call site of kmpc_task with the values received at the outlined body function. This is really complicated in the old approach, because there is no direct link possible between the captured values and the local copies in the thread (the stores go to some really hard to describe location and the body function reference is only at some point later).
In the new approach you link shared_and_private_vars to the argument the task function receives. Now you encapsulate the call site in an additional, modifiable level of indirection (see D71505), and the Attributor will unpack the struct (see D68852), propagate constants, alias information, ... between the task invocation and the task function.

Having a copy function per type allows us to reuse it, otherwise we have one copy function per static task location (at worst). Either works for me I think.

I also would be OK with either option.
But note that using per-type functions will require more additions to the interface, like:

(..., num_objects, array_of_copy_wrappers, array_of_desctuctor_wrappers, array_of_obj_offsets).

Then the library can iterate over objects to copy-construct them, then iterate to destroy them after the task is complete. Without any possibility of inlining of any wrappers.

Per-task function only needs two additions - copy_wrapper and destructor_wrapper, all other details can live inside them, including possible inlining of constructors and destructors.

In D71989#1822326, @AndreyChurbanov wrote:

Having a copy function per type allows us to reuse it, otherwise we have one copy function per static task location (at worst). Either works for me I think.

I also would be OK with either option.
But note that using per-type functions will require more additions to the interface, like:

(..., num_objects, array_of_copy_wrappers, array_of_desctuctor_wrappers, array_of_obj_offsets).

Then the library can iterate over objects to copy-construct them, then iterate to destroy them after the task is complete. Without any possibility of inlining of any wrappers.

Per-task function only needs two additions - copy_wrapper and destructor_wrapper, all other details can live inside them, including possible inlining of constructors and destructors.

Agreed. Interestingly, the "per-type" solution should allow you to implement the "per-task" version on top. You pretend all objects are part of a (task) meta-object which will copy them one by one.

@fghanim @kiranchandramohan @kiranktp @AMDChirag @anchu-rajendran @SouraVX @Meinersbur, Anyone interested in taking this over? The required changes in addition to the diff have been discussed with @AndreyChurbanov in the comments above.

In D71989#2455515, @jdoerfert wrote:

The required changes in addition to the diff have been discussed with @AndreyChurbanov in the comments above.

Actually the __kmpc_task function is going to be a bit more complicated in reality.

Because besides parameters for copy-construction + destruction of firstprivate objects, one will also need to pass to the __kmpc_task:

• priority value if any;
• affinity value(s) if any, probably single value is enough (this may be not supported yet, but apparently will be supported);
• address of event handle to be returned for detachable task, offset of the event handle in shared_and_private_vars so that the library can put corresponding address there if it is used inside the task code, or some indicator that library should not put the address there if it is not used inside the task code (e.g. negative offset?);
• offset of address of task structure in shared_and_private_vars for untied task, so that the compiler can generate __kmpc_omp_task calls to re-schedule partially executed task.

Also the __kmpc_task should better not call any __kmpc_* functions given recent activity of Joachim Protze (D92197) to eliminate all __kmpc_* calls from inside the library, and replace them with corresponding internal calls. Otherwise OMPT cannot reliably determine entry to / exit from the runtime library, IINM. But this might be a separate patch.

Please consider the special cases of task_if0, which preferably should use __kmpc_omp_task_with_deps:

• #pragma omp task if(0) depend(out:A) detach(event) : the detached undeferred task needs the out dependency to release the dependency only when detach is fulfilled
• #pragma omp task if(0) depend(inoutmutexset:A) : the undeferred task can execute mutually exclusive with any other inoutmutexset:A task.

I'm not sure that these are all the special cases.

See the bugs for full OpenMP code examples:

https://bugs.llvm.org/show_bug.cgi?id=46185
https://bugs.llvm.org/show_bug.cgi?id=46193

Hi @jdoerfert, we (BSC) may be able to work on this but we don't want to step on each one toes. Are there plans to push this forward (by you or someone else)?

How essential is changing the task interface in the context of your vision for the OpenMP-wise optimisations? Reusing the existing interface may not be ideal in that sense but could allow us to have a baseline already working for flang.

Thoughts? Thanks!

In D71989#2987095, @rogfer01 wrote:

Hi @jdoerfert, we (BSC) may be able to work on this but we don't want to step on each one toes. Are there plans to push this forward (by you or someone else)?

How essential is changing the task interface in the context of your vision for the OpenMP-wise optimisations? Reusing the existing interface may not be ideal in that sense but could allow us to have a baseline already working for flang.

Thoughts? Thanks!

I am not actively working on this, @ggeorgakoudis was interested though. We can communicate via email what is a good way forward.
I am fine with keeping the original interface first and just moving the code, if you think that is easier.
(I was hoping that API simplifications will make the move easier but I might have been wrong.)

Hi all, is this patch being worked on? I wanted to use this for adding support for task construct in flang.

In D71989#3413237, @shraiysh wrote:

Hi all, is this patch being worked on? I wanted to use this for adding support for task construct in flang.

I don't think it is. Read the discussion to see why this got stalled. Best way forward is to take this but *not* do the API changes I included.

In D71989#3413239, @jdoerfert wrote:

In D71989#3413237, @shraiysh wrote:

Hi all, is this patch being worked on? I wanted to use this for adding support for task construct in flang.

I don't think it is. Read the discussion to see why this got stalled. Best way forward is to take this but *not* do the API changes I included.

Okay, I am assuming a lot of the OpenMPIRBuilder.cpp code can be reused from here, but instead of creating the new proposed function, I should create the old functions with those arguments. I hope this is the correct approach, please correct me if I am wrong.

I wanted to know how to proceed about this? Do I borrow stuff from this patch and submit a new differential with you as a co-author? Do I push to this same differential? (Can we do that?) Please advise me on this. Thanks!

I wanted to know how to proceed about this? Do I borrow stuff from this patch and submit a new differential with you as a co-author? Do I push to this same differential? (Can we do that?) Please advise me on this. Thanks!

There is a "Commandeer Revision" option in the Add Action Menu. You can use that to take over this patch from @jdoerfert. Continuing here might have some advantages in the context, existing reviews and reviewers etc.

Thanks @shraiysh for taking up the work for task. This is the most important pending work for non-target OpenMP.

Could you expand the Summary a bit more with the approach taken and how it differs from current Clang task codegen?

I have added a few questions and comments.

FYI, I am revising how outlining works, still waiting on D115216 getting reviewed.

Ping for review.