This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP][deviceRTLs] Separate declaration of target dependent functions from `target_impl.h`
ClosedPublic

Authored by tianshilei1992 on Jan 23 2021, 6:36 PM.

Details

Summary

This patch created a new header file target_interface.h for declarations of all target dependent functions. All future targets can get things work by simply implementing all functions declared in the header and macros/data same as each target_impl.h.

Diff Detail

Event Timeline

tianshilei1992 created this revision.Jan 23 2021, 6:36 PM
tianshilei1992 requested review of this revision.Jan 23 2021, 6:36 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 23 2021, 6:36 PM
openmp/libomptarget/deviceRTLs/amdgcn/src/target_impl.h
79

All the declarations that moved to targwt_interface should be deleted from here (as well as from nvptx target_impl)

openmp/libomptarget/deviceRTLs/common/omptarget.h
333

Just INLINE?

openmp/libomptarget/deviceRTLs/target_interface.h
27

I guess these should be EXTERN, not extern DEVICE

'new header file target_impl.h' should be 'new header file target_interface.h' in the commit message

tianshilei1992 added inline comments.Jan 26 2021, 8:32 AM
openmp/libomptarget/deviceRTLs/common/omptarget.h
333

otherwise?

openmp/libomptarget/deviceRTLs/target_interface.h
27

They're C++ functions, so cannot use EXTERN directly which is expanded to extern "C" DEVICE.

Rebased and fixed comments

tianshilei1992 marked 3 inline comments as done.Jan 26 2021, 8:45 AM
tianshilei1992 edited the summary of this revision. (Show Details)
JonChesterfield accepted this revision.EditedJan 27 2021, 1:25 AM

Couple of minor points about redundant function annotations, otherwise good. Thanks for moving the amdgpu code out of the header.

I wonder if the kmpc_impl functions should be consistently extern C, at least when not overloaded.

openmp/libomptarget/deviceRTLs/common/omptarget.h
333

I mean, INLINE expands to a definition that includes DEVICE, so we don't need both

openmp/libomptarget/deviceRTLs/target_interface.h
27

Ah, so 'extern' here is dead. Functions are non-static by default.

This revision is now accepted and ready to land.Jan 27 2021, 1:25 AM

Updated comments

rebase to triger a new round of test

This revision was landed with ongoing or failed builds.Jan 28 2021, 5:14 AM
This revision was automatically updated to reflect the committed changes.
openmp/libomptarget/deviceRTLs/amdgcn/src/target_impl.hip
192 ↗(On Diff #319837)

I missed the language linkage change from c++ to c on these, fixed on main.