[openmp] Mark used variables as retain as well

[JonChesterfield]

Summary

D97446 changed the behaviour of 'used'. Compensate.

[JonChesterfield]

@ronlieb @gregrodgers I think this will fix the blocker for aomp if cherry-picked

[ronlieb]

LGTM,

[JonChesterfield]

I'm hopeful that D97446 can be adjusted so that attribute((used)) works out of the box, at which point we can consider reverting this. Until then, workaround seems reasonable.

[jdoerfert]

This might have broke nvidia and I'm not sure I understand the logic.

Why do we need retain? When is the variable being removed? I have seen problems w/ AMDGPU offloading when globals are hidden and then not exposed, is that the same problem?

[JonChesterfield]

See D97446. Used no longer means "used by something so don't delete it".

I don't know why we had no multiply defined symbol errors with the old version, should have done. Maybe lucky interleaving of the breaking change to the attribute with the changes to the driver.

Tests in trunk don't use the environment variable, so didn't notice it missing / containing zero. In this case it was 'used' being turned into compiler.used then internalised, so not quite the same as symbol visibility. Similar sort of thing though

[jdoerfert]

In D117211#3240933, @JonChesterfield wrote:

See D97446. Used no longer means "used by something so don't delete it".

I don't know why we had no multiply defined symbol errors with the old version, should have done. Maybe lucky interleaving of the breaking change to the attribute with the changes to the driver.

Tests in trunk don't use the environment variable, so didn't notice it missing / containing zero.

That doesn't help me. When is the variable be deleted?

[MaskRay]

In D117211#3240933, @JonChesterfield wrote:

See D97446. Used no longer means "used by something so don't delete it".

I don't know why we had no multiply defined symbol errors with the old version, should have done. Maybe lucky interleaving of the breaking change to the attribute with the changes to the driver.

Tests in trunk don't use the environment variable, so didn't notice it missing / containing zero. In this case it was 'used' being turned into compiler.used then internalised, so not quite the same as symbol visibility. Similar sort of thing though

I am afraid there is a confusion.

__attribute__((used)) did lower to llvm.used on ELF, but GNU ld/gold/ld.lld were always free to discard the section. Mach-O ld64 retains the section, though.

__attribute__((used)) lowers to llvm.compiler.used now, which has the same behavior as the old ELF llvm.used.

It is possible that some optimization passes do not respect llvm.compiler.used as Clang previously did not often emit llvm.compiler.used.

[JonChesterfield]

The section discard behaviour is opt-in though, so any program that wrote 'used' to retain the variable and then used gc-sections, discovered it didn't work, and worked around that. Probably by compromising on gc.

So we aren't going from a state of broken to better, we're going from a system that worked reliably for years (provided the linker wasn't told to deadstrip sections and the compiler wasn't told to put the global in it's own section) to one that silently discards symbols.

If the idea is llvm.used and llvm.compiler.used shall now have the same semantics, we can kill a long tail of bugs by deleting one of them.

[MaskRay]

In D117211#3241759, @JonChesterfield wrote:

The section discard behaviour is opt-in though, so any program that wrote 'used' to retain the variable and then used gc-sections, discovered it didn't work, and worked around that. Probably by compromising on gc.

So we aren't going from a state of broken to better, we're going from a system that worked reliably for years (provided the linker wasn't told to deadstrip sections and the compiler wasn't told to put the global in it's own section) to one that silently discards symbols.

If the idea is llvm.used and llvm.compiler.used shall now have the same semantics, we can kill a long tail of bugs by deleting one of them.

I am afraid that you may still have misunderstanding.

The patch series D97446 has moved ELF from a state of broken to better. It matches GCC. It is just different from traditional Mach-O Clang behavior.

The issue that some optimization passes may have problems is not an argument for not doing D97446.

I have mentioned on D97446 how the split of llvm.used and llvm.compiler.used is useful. I managed to 10+ object size decrease for some instrumentations.

openmp/libomptarget/DeviceRTL/src/Configuration.cpp

 using namespace _OMP;
 
 #pragma omp declare target
 
 extern uint32_t __omp_rtl_debug_kind; // defined by CGOpenMPRuntimeGPU
 
 // TODO: We want to change the name as soon as the old runtime is gone.
 DeviceEnvironmentTy CONSTANT(omptarget_device_environment)
-    __attribute__((used));
+    __attribute__((used, retain));
 
 uint32_t config::getDebugKind() {
   return __omp_rtl_debug_kind & omptarget_device_environment.DebugKind;
 }
 
 uint32_t config::getNumDevices() {
   return omptarget_device_environment.NumDevices;
 }

openmp/libomptarget/DeviceRTL/src/Utils.cpp

 #include "Mapping.h"
 
 #pragma omp declare target
 
 using namespace _OMP;
 
 namespace _OMP {
 /// Helper to keep code alive without introducing a performance penalty.
-__attribute__((used, weak, optnone)) void keepAlive() {
+__attribute__((used, retain, weak, optnone)) void keepAlive() {
   __kmpc_get_hardware_thread_id_in_block();
   __kmpc_get_hardware_num_threads_in_block();
   __kmpc_get_warp_size();
   __kmpc_barrier_simple_spmd(nullptr, 0);
   __kmpc_barrier_simple_generic(nullptr, 0);
 }
 } // namespace _OMP
 

openmp/libomptarget/deviceRTLs/amdgcn/src/target_impl.h

 #define PRIu64 "lu"
 
 typedef uint64_t __kmpc_impl_lanemask_t;
 
 #define INLINE inline
 #define NOINLINE __attribute__((noinline))
 #define ALIGN(N) __attribute__((aligned(N)))
 #define PLUGIN_ACCESSIBLE                                                      \
-  __attribute__((used)) /* Don't discard values the plugin reads */            \
+  __attribute__((used))   /* Don't discard values the plugin reads */          \
+  __attribute__((retain)) /* Also needed to keep values alive */               \
   __attribute__((visibility("default"))) /* Access via SHT_HASH */             \
   __attribute__((section(".data")))      /* Not .bss, can write before load */
 
 #include "llvm/Frontend/OpenMP/OMPGridValues.h"
 
 INLINE constexpr const llvm::omp::GV &getGridValue() {
   return llvm::omp::getAMDGPUGridValues<__AMDGCN_WAVEFRONT_SIZE>();
 }

openmp/libomptarget/deviceRTLs/common/src/support.cu

#include "common/generated_microtask_cases.gen"
   default:
     printf("Too many arguments in kmp_invoke_microtask, aborting execution.\n");
     __builtin_trap();
   }
 }
 
 namespace _OMP {
 /// Helper to keep code alive without introducing a performance penalty.
-__attribute__((used, weak, optnone)) void keepAlive() {
+__attribute__((used, retain, weak, optnone)) void keepAlive() {
   __kmpc_get_hardware_thread_id_in_block();
   __kmpc_get_hardware_num_threads_in_block();
   __kmpc_get_warp_size();
   __kmpc_barrier_simple_spmd(nullptr, 0);
   __kmpc_barrier_simple_generic(nullptr, 0);
 }
 } // namespace _OMP
 
 #pragma omp end declare target