[Openmp-commits] [PATCH] D43626: [OpenMP] Remove implicit data sharing using device shared memory from libomptarget

[Gheorghe-Teodor Bercea via Phabricator via Openmp-commits]

gtbercea added a comment.

In https://reviews.llvm.org/D43626#1027237, @Hahnfeld wrote:

> In https://reviews.llvm.org/D43626#1027103, @gtbercea wrote:
>
> > In https://reviews.llvm.org/D43626#1025517, @Hahnfeld wrote:
> >
> > > In https://reviews.llvm.org/D43626#1024135, @grokos wrote:
> > >
> > > > @Hahnfeld: Did our private discussion answer your questions about this plan?
> > >
> > >
> > > Partly, I'm still not happy about this turns out to be required after all of this was upstreamed. However if it's really needed you should at least make sure that basic applications continue to compile, something which we only reached recently.
> >
> >
> > Data sharing cannot be used today due to the missing LLVM NVPTX backend patch so it should not affect applications that compile right now. This patch and it's Clang counterpart need to be committed at the same time to ensure continuity.
>
>
> Got that but that doesn't justify upstreaming a part and kind of reverting it a few weeks later.


Reverting this is not part of the initial plan of course. I do agree with you that this is not ideal. We have been working on this problem over the past couple of weeks and have come up with a better solution to the data sharing problem using global memory. We always meant to use global memory as a more generic solution due to the fact that it doesn't have the size constraints that shared memory does. A global memory solution is also more complicated because there is a lot more legwork required by the compiler in order to share variables correctly. This is why the shared memory solution was there first since it was "easier" to rely on the properties of the GPU shared memory.

> 
> 
>>> In any case, the result of our private discussion needs to be made public. In particular I still don't see either explanation nor reasoning in any of the patches.
>> 
>> The shared memory scheme was never meant to cover all the cases in OpenMP and it was meant more as an optimization. For that to be true, the global memory scheme needs to be the default scheme used. The shared memory scheme can then be re-intorduced on top of the global memory scheme as an optimization.
> 
> Again: You should put that in the summary of all patches!




Repository:
  rOMP OpenMP

https://reviews.llvm.org/D43626