Page MenuHomePhabricator

[OpenMP] Remove implicit data sharing using device shared memory from libomptarget
ClosedPublic

Authored by gtbercea on Feb 22 2018, 8:43 AM.

Details

Summary

This patch reverts the changes to libomptarget that were coupled with the changes to Clang code gen for data sharing using shared memory. A similar patch exists for Clang: D43625

Shared memory is meant to be used as an optimization on top of a more general scheme. So far we didn't have a global memory implementation ready so shared memory was a solution which applied to the current level of OpenMP complexity supported by trunk on GPU devices (due to the missing NVPTX backend patch this functionality has never been exercised). Now that we have a global memory solution this patch is "in the way" and needs to be removed (for now). This patch (or an equivalent version of it) will be put out for review once the global memory scheme is in place.

Diff Detail

Repository
rOMP OpenMP

Event Timeline

gtbercea created this revision.Feb 22 2018, 8:43 AM
gtbercea edited the summary of this revision. (Show Details)Feb 22 2018, 3:55 PM

As far as I am concerned, I agree that the existing data sharing scheme should be removed in favour of a simpler future patch which will introduce the more generic data sharing scheme.

@Hahnfeld: Did our private discussion answer your questions about this plan?

gtbercea added a comment.EditedMar 1 2018, 10:58 AM

As far as I am concerned, I agree that the existing data sharing scheme should be removed in favour of a simpler future patch which will introduce the more generic data sharing scheme.

@Hahnfeld: Did our private discussion answer your questions about this plan?

I could post the combined patch, the one which combines this patch with the one that adds the new functions. The new functions will contain a naive implementation of those methods but one which would work correctly when the Clang code gen lands. I'm just putting this here as an alternative.

@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.

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.

@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.

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.

@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.

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!

@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!

@Hahnfeld any further questions on this?

@Hahnfeld any further questions on this?

Again: You should put that in the summary of all patches!

Can't be that hard! I won't accept this patch anyway because I'm not interested anymore in this data sharing functionality.

@Hahnfeld any further questions on this?

Again: You should put that in the summary of all patches!

Can't be that hard! I won't accept this patch anyway because I'm not interested anymore in this data sharing functionality.

Sorry for mistakenly including you in the list of reviewers.

@Hahnfeld any further questions on this?

Again: You should put that in the summary of all patches!

Can't be that hard! I won't accept this patch anyway because I'm not interested anymore in this data sharing functionality.

Sorry for mistakenly including you in the list of reviewers.

Can you still please update the summary of all patches as I suggested?

gtbercea edited the summary of this revision. (Show Details)Mar 7 2018, 9:20 AM
grokos accepted this revision.Mar 7 2018, 12:53 PM

I think the patch looks good. Let's remove the obsolete code in preparation for the new data-sharing scheme.

This revision is now accepted and ready to land.Mar 7 2018, 12:53 PM
This revision was automatically updated to reflect the committed changes.