[Openmp-commits] [PATCH] D53141: [OpenMP][libomptarget] Add runtime function for pushing coalesced global records

[Alexey Bataev via Phabricator via Openmp-commits]

ABataev added a subscriber: Hahnfeld.
ABataev added a comment.

In https://reviews.llvm.org/D53141#1262378, @Hahnfeld wrote:

> In https://reviews.llvm.org/D53141#1262019, @ABataev wrote:
>
> > In https://reviews.llvm.org/D53141#1261996, @Hahnfeld wrote:
> >
> > > I guess this will break the case when the `DataSize` passed to `__kmpc_data_sharing_push_stack()` needs additional alignment: With this change it is handled in `data_sharing_push_stack_common()` but `__kmpc_data_sharing_push_stack()` will determine `PushSize` without the adjustment and do the final pointer arithmetic.
> >
> >
> > Why `DataSize` might require an additional alignment? The `DataSize` must be already aligned.
>
>
> It's required in `__kmpc_data_sharing_push_stack()` for some cases, please see https://reviews.llvm.org/D52655.


It is another problem. The problem is that the `StackP` pointer is unaligned after all the pointer maths. The DataSize itself should not be aligned.

> Another problem with this patch is that `__kmpc_data_sharing_push_stack()` is adding the lane offset to the return value of `omptarget_nvptx_SimpleThreadPrivateContext::Allocate()`. AFAICS this will break `lastprivate` which requires the same buffer for all threads.
> 
> Anyway, you must do what you think to be correct.

Yes, there must be an additional check `size_t PushSize = (isRuntimeUninitialized() || IsMasterThread()) ? DataSize : WARPSIZE * DataSize;`


Repository:
  rOMP OpenMP

https://reviews.llvm.org/D53141