[OpenMP][NVPTX] Disable OpenMPOpt when building deviceRTLs

[tianshilei1992]

Summary

We build deviceRTLs with -O1 by default, which also triggers OpenMPOpt. When
the info cache is created, some attributes are removed. As a result, although we
mark a few functions noinline, they are still inlined when the bitcode library
is generated. This can cause an issue in middle end optimization.

[jdoerfert]

LGTM

openmp/libomptarget/deviceRTLs/nvptx/CMakeLists.txt

156

remove this line then.

[tianshilei1992]

fix comments

[JonChesterfield]

Does the same issue not affect application code? Or in other words, what was the issue in the middle end that disabling the pass fixes?

[tianshilei1992]

In D106710#2903223, @JonChesterfield wrote:

Does the same issue not affect application code? Or in other words, what was the issue in the middle end that disabling the pass fixes?

That issue will not affect application. The issue is, in OpenMPOpt, when we build the information cache, such attributes will be stripped. For example, although we add noinline attribute to the function __kmpc_parallel_level, it is stripped. As a result, the function call in __kmpc_parallel_51 is inlined. That is a problem when we optimize application code especially using attributor because we are using the information that the function call should be there. However, it is not a problem when we build the deviceRTLs as all OpenMP optimization must start with kernels, but when building the deviceRTLs there is no kernel so no optimization from OpenMPOpt is fired.
We disable OpenMPOpt when building deviceRTLs. It would not cause any problem as eventually it will be triggered along with application code.

[JonChesterfield]

Could you clarify:

all OpenMP optimization must start with kernels

Is the problem that the deviceRTL defines functions with names that are pattern matched from the optimisation pass, in which case this patch is definitely necessary? Analogous to building libc itself as ffreestanding.

I'm slightly surprised that we don't have an early return in the pass when there are no kernels in the IR module since that is probably quicker than mutating the module and then mutating it back. If we implemented that then we wouldn't need to special case the cmake here, but it might still be clearer to keep it explicitly disabled.

[jdoerfert]

In D106710#2904091, @JonChesterfield wrote:

I'm slightly surprised that we don't have an early return in the pass when there are no kernels in the IR module since that is probably quicker than mutating the module and then mutating it back.

We don't mutate and mutate back. We want to run on openmp-device modules without kernels to do things like globalization removal if the kernel is split across TUs, at least until we do LTO by default.

[JonChesterfield]

In D106710#2904210, @jdoerfert wrote:

In D106710#2904091, @JonChesterfield wrote:

I'm slightly surprised that we don't have an early return in the pass when there are no kernels in the IR module since that is probably quicker than mutating the module and then mutating it back.

We don't mutate and mutate back.

I was thinking of D106707, haven't looked for more examples

We want to run on openmp-device modules without kernels to do things like globalization removal if the kernel is split across TUs, at least until we do LTO by default.

OK, cool. If we have/want optimisations that run without visibility of the top level kernel then we don't want to skip the pass when there are no top level kernels.

I think all is good here, I just didn't understand the commit message.

[jdoerfert]

In D106710#2904241, @JonChesterfield wrote:

In D106710#2904210, @jdoerfert wrote:

In D106710#2904091, @JonChesterfield wrote:

I'm slightly surprised that we don't have an early return in the pass when there are no kernels in the IR module since that is probably quicker than mutating the module and then mutating it back.

We don't mutate and mutate back.

I was thinking of D106707, haven't looked for more examples

I'm confused. What has that to do with this? In the patch we preserve symbols that have been made internal in the earlier linking step so we can use them later, all that happens after the runtime is already build and optimized and shipped.

We want to run on openmp-device modules without kernels to do things like globalization removal if the kernel is split across TUs, at least until we do LTO by default.

OK, cool. If we have/want optimisations that run without visibility of the top level kernel then we don't want to skip the pass when there are no top level kernels.

I think all is good here, I just didn't understand the commit message.

[JonChesterfield]

The behaviour of the optimisation pass seems germane to disabling the optimisation pass.

[JonChesterfield]

openmp/libomptarget/deviceRTLs/nvptx/CMakeLists.txt

210

tangential to this, I think we should run opt -O2 or similar across the linked bitcode library before it ships, and internalize all the symbols that aren't __kmpc_ prefixed at the same time

[JonChesterfield]

amdgpu is still using openmp-opt-disable-internalization, going to patch that shortly

openmp/libomptarget/deviceRTLs/nvptx/CMakeLists.txt

set(cuda_src_files
   ${devicertl_common_directory}/src/sync.cu
   ${devicertl_common_directory}/src/task.cu
   ${devicertl_common_directory}/src/shuffle.cpp
   src/target_impl.cu
 )
 
 # Set flags for LLVM Bitcode compilation.
 set(bc_flags -S -x c++ -O1 -std=c++14
-             -mllvm -openmp-opt-disable-internalization
+             -mllvm -openmp-opt-disable
              -target nvptx64
              -Xclang -emit-llvm-bc
              -Xclang -aux-triple -Xclang ${aux_triple}
              -fopenmp -fopenmp-cuda-mode -Xclang -fopenmp-is-device
              -Xclang -target-feature -Xclang +ptx61
              -D__CUDACC__
              -I${devicertl_base_directory}
              -I${devicertl_common_directory}/include
foreach(src ${cuda_src_files})
     set_property(DIRECTORY APPEND PROPERTY ADDITIONAL_MAKE_CLEAN_FILES ${outfile})
 
     list(APPEND bc_files ${outfile})
   endforeach()
 
   set(bclib_name "libomptarget-nvptx-sm_${sm}.bc")
 
   # Link to a bitcode library.
   add_custom_command(OUTPUT ${CMAKE_CURRENT_BINARY_DIR}/${bclib_name}
       COMMAND ${bc_linker}
         -o ${CMAKE_CURRENT_BINARY_DIR}/${bclib_name} ${bc_files}
       DEPENDS ${bc_files}
       COMMENT "Linking LLVM bitcode ${bclib_name}"
   )
   if("${bc_linker}" STREQUAL "$<TARGET_FILE:llvm-link>")
     # Add a file-level dependency to ensure that llvm-link is up-to-date.
     # By default, add_custom_command only builds llvm-link if the