This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP][libomptarget] Add __tgt_target_return_t enum for __tgt_target_XXX return int
ClosedPublic

Authored by ye-luo on Sep 5 2021, 11:14 PM.

Details

Summary

The defintion of OFFLOAD_SUCCESS and OFFLOAD_FAIL used in plugin APIs and libomptarget public APIs are not consistent.
Create __tgt_target_return_t for libomptarget public APIs.

Diff Detail

Event Timeline

ye-luo created this revision.Sep 5 2021, 11:14 PM
ye-luo requested review of this revision.Sep 5 2021, 11:14 PM
Herald added a project: Restricted Project. · View Herald Transcript

I think an enum is great. We should remove the old macros though. I'm also not sure to change the behavior for the return rc cases in this patch.

ye-luo added a comment.EditedSep 6 2021, 6:50 PM

In my understanding of the return of __tgt_target_mapper __tgt_target_teams_mapper, when offload is disabled via environment variable, there are early returns to return OMP_TGT_FAIL. When actually offload to target device executes, handleTargetOutcome aborts if rc != OFFLOAD_SUCCESS, thus return value is always OMP_TGT_SUCCESS although return rc seems like being able to return OFFLOAD_FAIL.

OMP_TGT_FAIL is recoverable and the host fallback runs.
OFFLOAD_FAIL is generally unrecoverable and should be stopped by handleTargetOutcome.

ye-luo updated this revision to Diff 370981.Sep 6 2021, 7:03 PM

Added assertion.

tianshilei1992 added inline comments.Sep 7 2021, 7:10 PM
openmp/libomptarget/src/interface.cpp
384–386

What is this for?

ye-luo added inline comments.Sep 7 2021, 7:32 PM
openmp/libomptarget/src/interface.cpp
384–386

right now if rc != OFFLOAD_SUCCESS, handleTargetOutcome abort the execution. So once this assert line was reached, rc should be always OFFLOAD_SUCCESS and thus return OMP_TGT_SUCCESS is safe. If one day, the behavior of handleTargetOutcome changes, this assertion should catch unexpected inconsistency.

I had another look at this. It seems the assertion is just fine, we would return SUCCESS anyway right after assert(), so we are now double-checking that SUCCESS is indeed the correct value to return.

openmp/libomptarget/src/interface.cpp
24–34

This should be moved to include/omptarget.h - that's where we currently define OFFLOAD_SUCCESS and OFFLOAD_FAIL. OMP_TGT_SUCCESS and OMP_TGT_FAIL are the values which are checked to decided whether offloading has been successful or we should fallback to the host, so their definitions should reside in the header file that the outside world will include. Meanwhile, OFFLOAD_SUCCESS and OFFLOAD_FAIL are no longer used for inter-component communication, so we can move them to private.h or some new header file (do that as part of D109277).

ye-luo updated this revision to Diff 371997.Sep 10 2021, 1:08 PM

Move enum to header

ye-luo added inline comments.Sep 10 2021, 1:12 PM
openmp/libomptarget/src/interface.cpp
24–34

yes. I think after this patch OFFLOAD_SUCCESS/OFFLOAD_FAIL are only for libomptarget internal use, it will be shared between the plugin and libomptarget and thus needs to be on a separate header file instead of inside private.h. Will do the change as part of D109277

grokos accepted this revision.Sep 10 2021, 1:59 PM
This revision is now accepted and ready to land.Sep 10 2021, 1:59 PM
This revision was landed with ongoing or failed builds.Sep 10 2021, 2:11 PM
This revision was automatically updated to reflect the committed changes.