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

[ye-luo]

Summary

Return status will be expanded beyond success/failure.

[tianshilei1992]

LGTM. One nit.

openmp/libomptarget/include/omptarget.h

29

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

[grokos]

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]

openmp/libomptarget/include/omptarget.h

29

Oh, you are right!

[ye-luo]

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.

[tianshilei1992]

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

[ye-luo]

In D109277#2984460, @tianshilei1992 wrote:

__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

openmp/libomptarget/include/omptarget.h

 //===-------- omptarget.h - Target independent OpenMP target RTL -- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // Interface to be used by Clang during the codegen of a
 // target region.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef _OMPTARGET_H_
 #define _OMPTARGET_H_
 
 #include <deque>
 #include <stddef.h>
 #include <stdint.h>
 
 #include <SourceInfo.h>
 
-#define OFFLOAD_SUCCESS (0)
-#define OFFLOAD_FAIL (~0)
-
 #define OFFLOAD_DEVICE_DEFAULT -1
 
 // Don't format out enums and structs.
 // clang-format off
 
+/// return error status
+enum __tgt_error : int32_t {
+  OFFLOAD_SUCCESS = 0,
+  OFFLOAD_FAIL = ~0
+};
+
 /// Data attributes for each data reference used in an OpenMP target region.
 enum tgt_map_type {
   // No flags
   OMP_TGT_MAPTYPE_NONE            = 0x000,
   // copy data from host to device
   OMP_TGT_MAPTYPE_TO              = 0x001,
   // copy data from device to host
   OMP_TGT_MAPTYPE_FROM            = 0x002,