[Openmp-commits] [PATCH] D134396: [OpenMP][libomptarget] New plugin infrastructure and new CUDA plugin

[Kevin Sala Penadés via Phabricator via Openmp-commits]

kevinsala added inline comments.


================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPGridValues.h:59
   /// The size reserved for data in a shared memory slot.
-  const unsigned GV_Slot_Size;
+  unsigned GV_Slot_Size;
   /// The default value of maximum number of threads in a worker warp.
----------------
kevinsala wrote:
> jhuber6 wrote:
> > Is removing `const` really necessary here?
> We are changing some values at run-time (e.g., the maximum number of teams) on each device. But since that modification is at device initialization, I believe we could construct a new GridValue with the definitive value instead of modifying the fields. I'll revert those changes.
After checking it with more detail, I left the fields as non-const to prevent complicating the plugin side. We are changing the values of `GenericDevice::GridValues` during device initialization, but after the running the initialization list, inside the device constructor function. There are ways to change the values in the constructor function (e.g., re-initializing the grid value struct), but it's not very clean.

So we can remove the const qualifiers from struct members and declare the struct variable itself as const if we don't want their members to be modifiable. Currently, the grid values defined in `OMPGridValues.h` are already `constexpr` so there should not be any problem. Is that fine?


================
Comment at: openmp/libomptarget/plugins-nextgen/common/PluginInterface/CMakeLists.txt:14
+# Plugin Interface library.
+add_library(PluginInterface OBJECT PluginInterface.cpp GlobalHandler.cpp)
+
----------------
jhuber6 wrote:
> We should use `add_llvm_library` here. Consult the other plugins.
The `PluginInterface` is a CMake object library, which we use to build the plugin libraries, similar to `elf_common`.  That one also uses `add_library` instead of `add_llvm_library`. So shouldn't we keep using `add_library` for `PluginInterface` too?


================
Comment at: openmp/libomptarget/plugins-nextgen/common/PluginInterface/GlobalHandler.cpp:28-29
+  std::lock_guard<std::mutex> Lock(ELFObjectFilesMutex);
+  auto Search = ELFObjectFiles.find(Image.getId());
+  if (Search == ELFObjectFiles.end()) {
+    Expected<ELF64LEObjectFile> ExpectedELF =
----------------
jhuber6 wrote:
> An early exit would be cleaner here. Something like this should work as far as I know.
> ```
> if (ElfObjectFiles.count(Image.getId())
>   return ElfObjectFiles[Image.getId()];
> 
> ELF64LEObjectFile &Elf = ElfObjectFiles[Image.getId()]
> ...
> Elf = std::move(*ElfOrErr);
> ```
> I prefer names like `XOrErr` for expected types.
I'm going to invert my if conditional so I can make an early exit but checking the map only once.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134396