Headers in deviceRTLs are deeply coupled. This patch just did a
cleanup. AMDGCN part has not been touched now but will be done soon.
Details
- Reviewers
jdoerfert JonChesterfield ABataev grokos
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
openmp/libomptarget/deviceRTLs/target_interface.h | ||
---|---|---|
81 | should be EXTERN void __kmpc_impl_named_sync(uint32_t num_threads); |
I really like about half of this change.
Creating a target_interface.h containing the common parts of the two target_impl is good. Maybe better located under common/ but doesn't matter much.
Expanding nvptx_interface.h and amdgcn_interface.h to contain the extra bits of target_interface.h seems reasonable, but it means a lot of symbols and definitions are pasted into the top of interface.h. It is then no longer a clear description of the interface to the libary.
Previously, interface.h was not included in the source files. There are pros and cons to that, overall it's probably better to have it in scope for the files that are implementing the functions within it as it increases the chance of the prototype matching the implementation.
Minimising the number of includes is not as good. It means that dropping debug.h, or dropping the include of target_interface from it, will break a bunch of files. Better for each source that calls functions defined in a header to include that header directly. In practice, this probably means writing #include "target_interface"at the top of most of them. It's slightly more typing at the top of the source files in exchange for a build that doesn't break when removing apparently dead includes from headers.
I think my concerns amount to putting #include target_interface everywhere that currently has #include target_impl, and keeping stuff that is not part of the library interface out of interface.h.
Top of interface.h could change to:
#ifndef _INTERFACES_H_ #define _INTERFACES_H_ #include <stddef.h> #include <stdint.h> #ifdef __AMDGCN__ // probably variant at some point, same effect #define EXTERN extern "C" __attribute__((device)) typedef uint64_t __kmpc_impl_lanemask_t; typedef uint32_t omp_lock_t; /* arbitrary type of the right length */ EXTERN uint32_t __kmpc_amdgcn_gpu_num_threads(); #endif #ifdef __CUDACC__ #define EXTERN extern "C" __device__ typedef uint32_t __kmpc_impl_lanemask_t; typedef uint32_t omp_lock_t; /* arbitrary type of the right length */ #endif
That has the advantage of clarity, and the disadvantage of putting a bunch of code behind a macro. It'll cause the rocm compiler some grief as there are a bunch of extensions under amdgcn_interface.h at present.
openmp/libomptarget/deviceRTLs/common/debug.h | ||
---|---|---|
32 | A lot of files only compile after this change because they include debug and debug happens to include target_interface. I'd much rather all the files that use functions declared in target_interface continue to include the target_interface header directly. | |
34 | The amdgcn implementation is freestanding. At some point I moved the libc and libc++ headers until target_impl to make that work. This means it can build without libc++ and without libc, which is important as neither exist on the device and including bits of the host libc seems hazardous. | |
openmp/libomptarget/deviceRTLs/common/omptarget.h | ||
17 | lets lose the "debug.h" // debug comment | |
openmp/libomptarget/deviceRTLs/nvptx/src/nvptx_interface.h | ||
12–72 | stdint was a good thing, means the header is self contained. I.e. doesn't have to be fortunately included after stdint.h at the include site. | |
19 | if nvtpx wants <cassert> or <stdlib>, please put them here |
openmp/libomptarget/deviceRTLs/interface.h | ||
---|---|---|
19 | dropping those headers is a bad thing, this file makes heavy use of uint32_t style types |
openmp/libomptarget/deviceRTLs/nvptx/src/target_impl.cu | ||
---|---|---|
36 | Moving these into the implementation seems reasonable. We could also find a header other than target_impl.h to put them in, and drop them from the target specific part entirely. |
clang-tidy: error: 'common/device_environment.h' file not found [clang-diagnostic-error]
not useful