[Openmp-commits] [PATCH] D70971: [libomptarget] Build a minimal deviceRTL for amdgcn

[Jon Chesterfield via Phabricator via Openmp-commits]

JonChesterfield marked 2 inline comments as done.
JonChesterfield added inline comments.


================
Comment at: openmp/libomptarget/deviceRTLs/amdgcn/CMakeLists.txt:16-18
+  $ENV{HOME}/rocm/aomp
+  /opt/rocm/aomp
+  /usr/lib/rocm/aomp
----------------
jdoerfert wrote:
> ABataev wrote:
> > JonChesterfield wrote:
> > > JonChesterfield wrote:
> > > > ABataev wrote:
> > > > > JonChesterfield wrote:
> > > > > > ABataev wrote:
> > > > > > > We can't build the runtime with regular clang?
> > > > > > Sure, it builds fine with trunk. I'm using a local build of clang 9.0 on one machine and the openmp/fortran fork on a second.
> > > > > > 
> > > > > > The find package seems to be a list of anywhere that clang might plausibly be found. Copied straight out of our fork, happy to remove any entries you don't like the look of.
> > > > > > 
> > > > > > The NVPTX_CUDA_COMPILER_DIR doesn't seem especially appropriately named. I use the same compiler for nvptx and for amdgcn.
> > > > > Can we then just use the clang (I think, you proposed this for nvcc) to build the runtime? Why shall we lookup for `rocm/aomp`?
> > > > Which clang? CMAKE_CXX_COMPILER_DIR may resolve to gcc. Or do you mean introduce LIBOMPTARGET_AMDGCN_CUDA_COMPILER_DIR and expect the user to match that up to a clang build?
> > > rocm/aomp is a likely location to find clang if using the rocm developer tools stack, and I think it gets installed into different places depending on whether it's from apt-get or github.
> > > 
> > > I'm a little unclear on how in tree compiler plus out of tree library (e.g. libc, opencl runtime, or the cuda toolchain) is supposed to work logistically.
> > Freshly built clang which is used to build `.bc` library for nvptx. Plus, if need it in some cases, I think it would be good to have a cmake configuration option for the user so he could point to his own installation of AOMP. Thoughts?
> Take the cmake flag that is used to build the .bc file of the library. 
There's some discussion on another diff about building libomp independently of the compiler so there may not be a freshly built clang available.

I've just had a go at tracing the logic for deciding what directory will be used for clang and it appears to be LLVM_INSTALL_PREFIX if set, otherwise LLVM_BUILD_BINARY_DIR, where find_package looks in:
- the AOMP environment variable (I think our tests use this)
- various install places
- wherever the nvptx compiler is (I'm OK with dropping this)
- the CXX compiler dir (one of my local builds uses this)

It's a bit of a mess but looks like canonical cmake to me.


================
Comment at: openmp/libomptarget/deviceRTLs/amdgcn/src/device_environment.h:18-26
+struct omptarget_device_environmentTy {
+  int32_t debug_level;   // gets value of envvar LIBOMPTARGET_DEVICE_RTL_DEBUG
+                         // only useful for Debug build of deviceRTLs 
+  int32_t num_devices;   // gets number of active offload devices 
+  int32_t device_num;    // gets a value 0 to num_devices-1
+};
+
----------------
ABataev wrote:
> JonChesterfield wrote:
> > ABataev wrote:
> > > JonChesterfield wrote:
> > > > ABataev wrote:
> > > > > Agan this thing. Can we make it common if it is really required? Again, why do we need these new fields? Could you remove it from the current patch?
> > > > It's different on nvptx and amdgcn, so not a great candidate for common. The extra fields are used to implement a couple of functions that are stubs in trunk which you weren't sure are necessary for nvptx.
> > > Maybe, it is better to exclude this from this patch? Maybe it would be good to have these fields for nvptx too or we don't need them to for amdgcn. Could you remind where we discussed this structure?
> > Discussion was in D69424. I could drop the file from this patch, but it's included by debug.h so that'll break the amdgcn build
> Ah, found it. Again, the same question. These fields are required to add 2 new `omp_...` functions. The question is the same. Are they required by the standard? If yes, we must have the same fields for nvptx. Otherwise, I don't think we need these fields at all.
"When called from a target region the effect of this routing is unspecified"

So nvptx returning zero is fine, and andgcn returning a meaningful value is also fine, as far as the standard is concerned.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70971/new/

https://reviews.llvm.org/D70971