-
Notifications
You must be signed in to change notification settings - Fork 18k
[OFFLOAD] Build DeviceRTL with SPIRV backend #174675
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 19 commits
996c2e1
5e746a8
3587050
84def59
5b6f225
dc78bad
e98cf56
40dfa28
2922a82
a18c339
ae099ba
db04823
802a65e
f2dbfa2
101fccd
7584cad
a660034
b79e42a
488209c
c5ac596
6366e0c
e090887
d50579f
7cc3270
af38777
4bd07fd
4ad7138
e66a4e4
867d96a
f92b77e
7e5e695
bb719be
b9875fd
e2cc882
5e20646
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -25,14 +25,18 @@ set(src_files | |
| ${CMAKE_CURRENT_SOURCE_DIR}/src/Workshare.cpp | ||
| ) | ||
|
|
||
| list(APPEND compile_options -flto) | ||
| if(NOT "${LLVM_DEFAULT_TARGET_TRIPLE}" MATCHES "^spirv" AND | ||
| NOT "${CMAKE_CXX_COMPILER_TARGET}" MATCHES "^spirv") | ||
| list(APPEND compile_options -flto) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should be using
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fixed |
||
| endif() | ||
| list(APPEND compile_options -fvisibility=hidden) | ||
| list(APPEND compile_options -Wno-unknown-cuda-version) | ||
| list(APPEND compile_options -nogpulib) | ||
| list(APPEND compile_options -nostdlibinc) | ||
| list(APPEND compile_options -fno-rtti) | ||
| list(APPEND compile_options -fno-exceptions) | ||
| list(APPEND compile_options -fconvergent-functions) | ||
| list(APPEND compile_options -Wno-unknown-cuda-version) | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unnecessary reordering
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed |
||
| if(LLVM_DEFAULT_TARGET_TRIPLE) | ||
| list(APPEND compile_options --target=${LLVM_DEFAULT_TARGET_TRIPLE}) | ||
| endif() | ||
|
|
@@ -52,6 +56,10 @@ elseif("${LLVM_DEFAULT_TARGET_TRIPLE}" MATCHES "^nvptx" OR | |
| "${CMAKE_CXX_COMPILER_TARGET}" MATCHES "^nvptx") | ||
| set(target_name "nvptx") | ||
| list(APPEND compile_options --cuda-feature=+ptx63) | ||
| elseif("${LLVM_DEFAULT_TARGET_TRIPLE}" MATCHES "^spirv64" OR | ||
| "${CMAKE_CXX_COMPILER_TARGET}" MATCHES "^spirv64") | ||
| set(target_name "spirv") | ||
| list(APPEND compile_options -emit-llvm -c) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You should be setting the link options to
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fixed |
||
| endif() | ||
|
|
||
| # Trick to combine these into a bitcode file via the linker's LTO pass. | ||
|
|
@@ -74,8 +82,15 @@ target_include_directories(libompdevice PRIVATE | |
| ${CMAKE_CURRENT_SOURCE_DIR}/../../libc | ||
| ${CMAKE_CURRENT_SOURCE_DIR}/../../offload/include) | ||
| target_compile_options(libompdevice PRIVATE ${compile_options}) | ||
| target_link_options(libompdevice PRIVATE | ||
| "-flto" "-r" "-nostdlib" "-Wl,--lto-emit-llvm") | ||
| if(NOT "${LLVM_DEFAULT_TARGET_TRIPLE}" MATCHES "^spirv" AND | ||
| NOT "${CMAKE_CXX_COMPILER_TARGET}" MATCHES "^spirv") | ||
| target_link_options(libompdevice PRIVATE | ||
| "-flto" "-r" "-nostdlib" "-Wl,--lto-emit-llvm") | ||
| else() | ||
| target_link_options(libompdevice PRIVATE | ||
| "-nostdlib" "-emit-llvm" "-Wl") | ||
|
jhuber6 marked this conversation as resolved.
Outdated
fineg74 marked this conversation as resolved.
Outdated
|
||
| endif() | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why aren't we building the device RTL in LTO, as we do for the rest?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. SPIRV backend doesn't support lto
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hm, what is the issue? We rely on LTO for performance, and without I doubt we can be competitive.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The library will get linked in and optimized somehow I'm sure, but the library itself gets no individual optimization step together. That definitely hurts performance, but the individual files are optimized so it's likely not catastrophic.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The problem is currently SPIR-V uses an out of tree linker |
||
|
|
||
| if(LLVM_DEFAULT_TARGET_TRIPLE) | ||
| target_link_options(libompdevice PRIVATE "--target=${LLVM_DEFAULT_TARGET_TRIPLE}") | ||
| endif() | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -131,7 +131,14 @@ struct IdentTy { | |
|
|
||
| using __kmpc_impl_lanemask_t = LaneMaskTy; | ||
|
|
||
| using ParallelRegionFnTy = void *; | ||
| #ifdef __SPIRV__ | ||
| const uint32_t ProgramAS = 9; | ||
| using FnPtrTy = __attribute__((address_space(ProgramAS))) void *; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. C++ attributes please
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Using attributes in [[]] form causes compilation error
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes. FnPtrTy is used as a type since SPIRV backend has issues that are related to address space of parameters. Since the issue is related to SPIRV only and to minimize code changes FnPtrTy was introduced as a type of pointer with attribute for SPIRV, and as a regular pointer for the rest. The correct use of c++ attributes requires to put it after a variable, while attribute can be put before type which makes it easier to use.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This deserves a comment, I mean the AS stuff above.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added comments in the code
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do the C++ attributes not work? And just make this constexpr or put the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The C++ attributes do not work because in this case they are placed in incorrect place. I believe we have discussed it couple of weeks back. I replaced the constant with the actual value. Regarding why we need a special address space for function better to check with @sarnex as he originally introduced it.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This doesn't work? https://godbolt.org/z/T94q8jz5q
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That worked. Thanks, fixed |
||
| #else | ||
| using FnPtrTy = void *; | ||
| #endif | ||
|
|
||
| using ParallelRegionFnTy = FnPtrTy; | ||
|
Comment on lines
+134
to
+144
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. C++ style attributes |
||
|
|
||
| using CriticalNameTy = int32_t[8]; | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,7 +16,17 @@ | |
|
|
||
| namespace ompx { | ||
|
|
||
| // SPIR-V backend does not support variadic functions except for | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have #175076 up but will need some SPIR-V people to look at it. |
||
| // __spirv_ocl_printf. This is to provide a workaround to use | ||
| // regular printf that is used in the code. | ||
| #if defined(__SPIRV__) | ||
| template <size_t N, typename... Args> | ||
| int printf(const char (&Format)[N], Args... args) { | ||
| return __spirv_ocl_printf(Format, args...); | ||
| } | ||
| #else | ||
| int printf(const char *Format, ...); | ||
| #endif | ||
|
|
||
| } // namespace ompx | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -31,18 +31,21 @@ extern "C" { | |
| for (size_t I = 0; I < count; ++I) | ||
| dstc[I] = C; | ||
| } | ||
|
|
||
| #if !defined(__SPIRV__) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hopefully my PR will resolve this |
||
| [[gnu::weak]] int printf(const char *Format, ...) { | ||
| __builtin_va_list vlist; | ||
| __builtin_va_start(vlist, Format); | ||
| return ::vprintf(Format, vlist); | ||
| } | ||
| #endif | ||
| } | ||
|
|
||
| #if !defined(__SPIRV__) | ||
| namespace ompx { | ||
| [[clang::no_builtin("printf")]] int printf(const char *Format, ...) { | ||
| __builtin_va_list vlist; | ||
| __builtin_va_start(vlist, Format); | ||
| return ::vprintf(Format, vlist); | ||
| } | ||
| } // namespace ompx | ||
| #endif | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -258,6 +258,63 @@ void setCriticalLock(omp_lock_t *Lock) { setLock(Lock); } | |
| #endif | ||
| ///} | ||
|
|
||
| #if defined(__SPIRV__) | ||
|
|
||
| uint32_t atomicInc(uint32_t *Address, uint32_t Val, atomic::OrderingTy Ordering, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This bit was changed in a previous PR to use the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Copied the implementation for spirv backend until backend is fixed |
||
| atomic::MemScopeTy MemScope) { | ||
| uint32_t old; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Style,
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed |
||
| while (true) { | ||
| old = atomic::load(Address, Ordering, MemScope); | ||
| if (old >= Val) { | ||
| if (atomic::cas(Address, old, 0, Ordering, Ordering)) | ||
| break; | ||
| } else if (atomic::cas(Address, old, old + 1, Ordering, Ordering)) | ||
| break; | ||
| } | ||
| return old; | ||
| } | ||
|
|
||
| void namedBarrierInit() {} // TODO | ||
| void namedBarrier() {} // TODO | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you trap here for now?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed |
||
| void fenceTeam(atomic::OrderingTy Ordering) { | ||
| return __scoped_atomic_thread_fence(Ordering, atomic::workgroup); | ||
| } | ||
|
|
||
| void fenceKernel(atomic::OrderingTy Ordering) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I should probably make these common, I thought I did that already.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I made #177710 hopefully this will let us simplify some of this if it lands without issue |
||
| return __scoped_atomic_thread_fence(Ordering, atomic::device); | ||
| } | ||
|
|
||
| void fenceSystem(atomic::OrderingTy Ordering) { | ||
| return __scoped_atomic_thread_fence(Ordering, atomic::system); | ||
| } | ||
|
|
||
| void syncWarp(__kmpc_impl_lanemask_t mask) { __gpu_sync_lane(mask); } | ||
| void syncThreads(atomic::OrderingTy Ordering) { __gpu_sync_threads(); } | ||
| void unsetLock(omp_lock_t *Lock) { | ||
| atomic::store((int32_t *)Lock, 0, atomic::seq_cst); | ||
| } | ||
| int testLock(omp_lock_t *Lock) { | ||
| return atomic::add((int32_t *)Lock, 0, atomic::seq_cst); | ||
| } | ||
| void initLock(omp_lock_t *Lock) { unsetLock(Lock); } | ||
| void destroyLock(omp_lock_t *Lock) { unsetLock(Lock); } | ||
| void setLock(omp_lock_t *Lock) { | ||
| int32_t *lock_ptr = (int32_t *)Lock; | ||
| bool acquired = false; | ||
| int32_t expected; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Style. Please review your patches for style before posting them.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed |
||
| while (!acquired) { | ||
| expected = 0; | ||
| if (expected == atomic::load(lock_ptr, atomic::seq_cst)) | ||
| acquired = | ||
| atomic::cas(lock_ptr, expected, 1, atomic::seq_cst, atomic::seq_cst); | ||
| } | ||
| } | ||
|
|
||
| void unsetCriticalLock(omp_lock_t *Lock) { unsetLock(Lock); } | ||
| void setCriticalLock(omp_lock_t *Lock) { setLock(Lock); } | ||
| void syncThreadsAligned(atomic::OrderingTy Ordering) { syncThreads(Ordering); } | ||
| #endif | ||
|
|
||
| } // namespace impl | ||
|
|
||
| void synchronize::init(bool IsSPMD) { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't belong here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This belongs next to the usage
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed