[PATCH] D113359: [Libomptarget][WIP] Introduce VGPU Plugin

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added inline comments.


================
Comment at: clang/lib/Driver/ToolChains/Gnu.cpp:3082
+  if (getTriple().getVendor() == llvm::Triple::OpenMP_VGPU) {
+    std::string BitcodeSuffix = "x86_64-vgpu";
+    clang::driver::tools::addOpenMPDeviceRTL(getDriver(), DriverArgs, CC1Args,
----------------
tianshilei1992 wrote:
> Maybe `"x86_64-openmp_vpu"` now?
not x86, right? triple contains the proper arch


================
Comment at: openmp/libomptarget/DeviceRTL/src/Mapping.cpp:29
+
+#include "ThreadEnvironment.h"
+
----------------
Move up to the beginning.


================
Comment at: openmp/libomptarget/DeviceRTL/src/Synchronization.cpp:291
+
+#include "ThreadEnvironment.h"
+namespace impl {
----------------
Move up.


================
Comment at: openmp/libomptarget/DeviceRTL/src/Synchronization.cpp:342
+  VGPUImpl::setLock((uint32_t *)Lock, UNSET, SET, OMP_SPIN,
+                    mapping::getBlockId(), atomicCAS);
+}
----------------
We should simply use omp locks. Either here, or maybe better, in VGPUImpl. So redirect all calls to there and use a proper lock. no OMP_SPIN and stuff


================
Comment at: openmp/libomptarget/DeviceRTL/src/Utils.cpp:118
+
+#include "ThreadEnvironment.h"
+namespace impl {
----------------
Move up


================
Comment at: openmp/libomptarget/DeviceRTL/src/Utils.cpp:127
+  return getThreadEnvironment()->shuffleDown(Var, Delta);
+}
+
----------------
Pass the mask, both times.


================
Comment at: openmp/libomptarget/plugins/vgpu/src/ThreadEnvironment.cpp:49
+  } // wait for 0 to be the read value
+}
+
----------------
see above.


================
Comment at: openmp/libomptarget/src/rtl.cpp:97
+      continue;
+    }
+
----------------
Not only x86, also let's not do strcmp. Extend RTLNAmes to be an array of structs with more elaborate information, e.g., is host flag. That said, unsure if not loading the plugin is the right way to not grab the image. Good enough for now.


================
Comment at: openmp/libomptarget/test/CMakeLists.txt:23
+    continue()
+  ENDIF()
   string(STRIP "${CURRENT_TARGET}" CURRENT_TARGET)
----------------
This is to disable the tests? Not sure this is a good way though. For one, can we check against -vgpu not x86, also openmp-vgpu or something, right?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113359