[Openmp-commits] [PATCH] D113359: [Libomptarget][WIP] Introduce VGPU Plugin

[Jon Chesterfield via Phabricator via Openmp-commits]

JonChesterfield added a comment.

I can't see it in the diff - does the cmake somewhere enable the existing tests on this new target?

A bit surprised to see ffi involved, are we thinking of spawning a separate process for the target?



================
Comment at: clang/lib/Basic/Targets/X86.h:49
 
+static const unsigned X86VGPUAddrSpaceMap[] = {
+    0,   // Default
----------------
It's not clear to me what this is x86 specific. Being able to run our tests on power / arm etc seems like an advantage. Would also mean we would avoid adding openmp stuff the x86 specific files. Maybe OpenMPVGPUAddrSpaceMap and put it in one of the openmp source files?


================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:3988
+                        (T.isNVPTX() || T.isAMDGCN() ||
+                         T.getVendor() == llvm::Triple::OpenMP_VGPU) &&
                         Args.hasArg(options::OPT_fopenmp_cuda_mode);
----------------
Add a isOpenmpVGPU function?


================
Comment at: openmp/libomptarget/DeviceRTL/CMakeLists.txt:135
              -I${devicertl_base_directory}/../include
+             -I${devicertl_base_directory}/../plugins/vgpu/src
              ${LIBOMPTARGET_LLVM_INCLUDE_DIRS_DEVICERTL}
----------------
Should only add this include to the vgu, not all the plugins. May be able to use relative include paths to drop it entirely


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113359