Page MenuHomePhabricator

[OpenMP][libomptarget] Change internal return status to enum type
AcceptedPublic

Authored by ye-luo on Sep 4 2021, 1:04 PM.

Details

Summary

Return status will be expanded beyond success/failure.

Diff Detail

Unit TestsFailed

TimeTest
610 msx64 debian > libomp.lock::omp_init_lock.c
Script: -- : 'RUN: at line 1'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang -fopenmp -pthread -fno-experimental-isel -I /var/lib/buildkite-agent/builds/llvm-project/openmp/runtime/test -I /var/lib/buildkite-agent/builds/llvm-project/build/projects/openmp/runtime/src -L /var/lib/buildkite-agent/builds/llvm-project/build/lib -I /var/lib/buildkite-agent/builds/llvm-project/openmp/runtime/test/ompt /var/lib/buildkite-agent/builds/llvm-project/openmp/runtime/test/lock/omp_init_lock.c -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/openmp/runtime/test/lock/Output/omp_init_lock.c.tmp -lm -latomic && /var/lib/buildkite-agent/builds/llvm-project/build/projects/openmp/runtime/test/lock/Output/omp_init_lock.c.tmp

Event Timeline

ye-luo created this revision.Sep 4 2021, 1:04 PM
ye-luo requested review of this revision.Sep 4 2021, 1:04 PM
Herald added a project: Restricted Project. · View Herald Transcript
tianshilei1992 accepted this revision.Sep 5 2021, 1:15 PM

LGTM. One nit.

openmp/libomptarget/include/omptarget.h
29

What about enum tgt_return_value : uint32_t, and set OFFLOAD_FAIL = ~0U?

This revision is now accepted and ready to land.Sep 5 2021, 1:15 PM
grokos added a subscriber: grokos.Sep 5 2021, 3:04 PM
grokos added inline comments.
openmp/libomptarget/include/omptarget.h
29

uint32_t may sound nicer, but keep in mind that these values are used as return values for __tgt_target_* (clang/libomptarget) and __tgt_rtl_* (libomptarget/plugins) functions, whose signatures indicate that they should return int - not even int32_t. In this context, I think it's better to change the enum type to pure int.

tianshilei1992 added inline comments.Sep 5 2021, 3:24 PM
openmp/libomptarget/include/omptarget.h
29

Oh, you are right!

ye-luo added a comment.Sep 5 2021, 8:48 PM

Luckily I sniffed a can of worms and put up this patch.

I grep "omptarget.h" and "OFFLOAD_SUCCESS" through out the repository and found zero hit outside libomptarget directory.
So I think no matter whatever value is returned via __tgt_target_* as int, the value is ignored.

Here is the list of __tgt_target funciton which returns int.

openmp/libomptarget/include/omptarget.h:int __tgt_target_nowait(int64_t device_id, void *host_ptr, int32_t arg_num,
openmp/libomptarget/include/omptarget.h:int __tgt_target_mapper(ident_t *loc, int64_t device_id, void *host_ptr,
openmp/libomptarget/include/omptarget.h:int __tgt_target_nowait_mapper(ident_t *loc, int64_t device_id, void *host_ptr,
openmp/libomptarget/include/omptarget.h:int __tgt_target_teams(int64_t device_id, void *host_ptr, int32_t arg_num,
openmp/libomptarget/include/omptarget.h:int __tgt_target_teams_nowait(int64_t device_id, void *host_ptr,
openmp/libomptarget/include/omptarget.h:int __tgt_target_teams_mapper(ident_t *loc, int64_t device_id, void *host_ptr,
openmp/libomptarget/include/omptarget.h:int __tgt_target_teams_nowait_mapper(

All those without _mapper calls a version with _mapper and it always call handleTargetOutcome before return.
This function stops the code based on the TargetOffloadPolicy.

@grokos could you confirm that the return to outside libomptarget is not used?
if this is the case, I'm in favor of use int32_t as tgt_rtl_* have and update all internal function targetDataBegin/targetDataEnd/targetDataUpdate/target return type to enum.
leave
tgt_target_* return type as int just for backward compatibility.

__tgt_target series is emitted by the compiler, and the return value is also checked. If the offloading fails, it calls a host version. Currently if offloading is mandatory, the program immediately abort in libomptarget. That being said, the return value of those interface functions are being used.

Changing the return value of internal functions to enum doesn’t hurt anything, but it is not beneficial as well because at the end of the day, it still has to return an integer of type int. Since the return value is communicated among multiple C interfaces, defining them as macro or enumeration or even global values doesn’t make too much difference.

If you want a cleaner way, I think we can define a set of return values for plug-in, another set of return values for those libomptarget interfaces, and a set of return values for internal functions. The first two sets have to be integer as they are for C functions. The last one can be anything you want, enum, enum class, or even a real class.

__tgt_target series is emitted by the compiler, and the return value is also checked. If the offloading fails, it calls a host version. Currently if offloading is mandatory, the program immediately abort in libomptarget. That being said, the return value of those interface functions are being used.

My puzzle is how the compiler generated callers understand what are the meanings of return value. It seems like based on an agreement without any communication.

If you want a cleaner way, I think we can define a set of return values for plug-in, another set of return values for those libomptarget interfaces, and a set of return values for internal functions. The first two sets have to be integer as they are for C functions. The last one can be anything you want, enum, enum class, or even a real class.

  1. return values for plug-in
  2. libomptarget interface
  3. internal functions

I found 1 and 2 actually doesn't mean precisely the same thing. So I took out D109304. For simplicity, I will using the same enum for 1 and 3.

So keep 1 as it was int32_t. keep 2 as it was int.
Then change 3 from int to int32_t if not already on int32_t