[Openmp-commits] [PATCH] D51107: [LIBOMPTARGET] Add support for mapping of lambda captures.

[Jonas Hahnfeld via Phabricator via Openmp-commits]

Hahnfeld added a comment.

In https://reviews.llvm.org/D51107#1211932, @AlexEichenberger wrote:

> In https://reviews.llvm.org/D51107#1211035, @Hahnfeld wrote:
>
> >
>
>
>
>
> > I agree that this doesn't work with the current code. IIRC the reason is that libomptarget has two loops dealing with `map` clauses: one in `target_data_begin` (which also handles `MEMBER_OF`) and the one in `target` which this patch is modifying. The problem is that `firstprivate` arguments are handled in the second, so the one in `target_data_begin` (which runs first) doesn't have a device address to resolve entries with `MEMBER_OF`. IMO the proper solution would be to have all allocations in one single loop in `target_data_begin`. The loop in `target` should only deal with constructing the arguments passed to the device.
> > 
> > To implement this efficiently we need a temporary data structure for each mapping so that we can store the device address allocated in `target_data_begin`. That pointer could then be queried in `target` to populate `tgt_args`.
> >  (Ideally the data structure would wrap each entry in the `args_base`, `args`, `arg_sizes`, and `arg_types` arrays into a nice object. This would have functions to hide all this bitmasking to find out the type.)
> > 
> > Long story short, I think that lambdas are indeed `firstprivate`. What needs to be fixed are `firstprivate` "mappings" together with `MEMBER_OF` (@AlexEichenberger  this already works in lomp, right?).
>
> While I am not 100% familiar with the libomptarget code yet, it is possible to handle the beginning of a target construct like a target begin (phase 1) , and the end of a target construct like a target end (phase 3), with the invocation of the target being phase 2. Keeping a data structure that hold the data from phase 1 to phase 3 is quite advantageous, as locating maps is generally expensive. Flow of info from phase 1 to phase 2 is minimal (only the list of parameters, which are indicated by a flag in the arg_types parameter.


Yes, that's how it is implemented in libomptarget. In fact it's the "problem" with the two loops that I described above: `firstprivate` "mappings" are not handled by phase 1, but instead in `target` (phase 2) which is why `MEMBER_OF` doesn't work.

> Incidentally, having a data structure becomes necessary once async is implemented, as there are then a lot more state to remember (e.g. events associated with the termination of specific data transfers, book keeping for deleting temporary buffers once data is copied over...)

That's one of my motiviation why I think this refactoring is worth it ;-)

> In my mind, MEMBER_OF does not imply MAP, it can be used for firstprivate just the same (though member_of firstprivate cannot be given by a user, as only map can have alloc of a whole structure, with to, from, tofrom for subsets of the structure).

... and that's the point that should be fixed in libomptarget. For now I think it's acceptable to do more expensive lookups before implementing the caching information.


Repository:
  rOMP OpenMP

https://reviews.llvm.org/D51107