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

[Joseph Huber via Phabricator via llvm-commits]

jhuber6 added a comment.

Thanks for doing this, we definitely need to make this interface more generic. here's some points on a first pass.



================
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.
----------------
Is removing `const` really necessary here?


================
Comment at: openmp/libomptarget/include/Utilities.h:149
+/// supported by C++17 and beyond.
+using StatusCode = StatusCode_<void>;
+
----------------
This looks like a re-implementation of [LLVM's error handling](https://llvm.org/docs/ProgrammersManual.html#recoverable-errors). is there a reason we need to use this? Otherwise it would be better to use existing LLVM libraries wherever we can.


================
Comment at: openmp/libomptarget/include/Utilities.h:166
+template <typename Ty> class Envar {
+  std::string Name;
+  Ty Data;
----------------
We should be able to use a `StringRef` here.


================
Comment at: openmp/libomptarget/plugins-nextgen/common/PluginInterface/CMakeLists.txt:14
+# Plugin Interface library.
+add_library(PluginInterface OBJECT PluginInterface.cpp GlobalHandler.cpp)
+
----------------
We should use `add_llvm_library` here. Consult the other plugins.


================
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 =
----------------
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.


================
Comment at: openmp/libomptarget/plugins-nextgen/common/PluginInterface/GlobalHandler.cpp:33
+    if (!ExpectedELF) {
+      INFO(OMP_INFOTYPE_DATA_TRANSFER, Device.getDeviceId(),
+         "Unable to open ELF image.");
----------------
This won't work, you need to consume the error either via `consumeError` or `toString`. And is this really something we emit as info? This is an error message, info messages are for usage statistics and other information the user wants to know during the normal execution of the program.


================
Comment at: openmp/libomptarget/plugins-nextgen/common/PluginInterface/GlobalHandler.cpp:56
+  if (SC) {
+    INFO(OMP_INFOTYPE_DATA_TRANSFER, Device.getDeviceId(),
+         "Failed to read global symbol metadata for '%s' from the device",
----------------
This should be an error message or a debug message, not an info message. Same for all the uses below.


================
Comment at: openmp/libomptarget/plugins-nextgen/common/PluginInterface/GlobalHandler.cpp:121
+
+  // TODO: We should improve the search by consulting HASH or GNU_HASH tables.
+  for (auto &SymbolIt : ELF->symbols()) {
----------------
I recently added a function for this in `ELFSymbols.cpp`, we should use that instead as it uses the hash tables if present and avoids duplicating code.
```
Expected<const typename ELF64LE::Sym *>
getELFSymbol(const ELFObjectFile<ELF64LE> &ELFObj, StringRef Name);
```


================
Comment at: openmp/libomptarget/plugins-nextgen/common/PluginInterface/GlobalHandler.h:41
+  // holding a private copy of the name as a std::string
+  std::string Name;
+  uint32_t Size;
----------------
This should be able to be a `StringRef` as well.


================
Comment at: openmp/libomptarget/plugins-nextgen/common/PluginInterface/GlobalHandler.h:105
+  /// Map to store the ELF object files that have been loaded
+  std::unordered_map<int32_t, ELF64LEObjectFile> ELFObjectFiles;
+  std::mutex ELFObjectFilesMutex;
----------------



================
Comment at: openmp/libomptarget/plugins-nextgen/common/PluginInterface/GlobalHandler.h:116-119
+  StatusCode moveGlobalBetweenDeviceAndHost(GenericDeviceTy &Device,
+                                         DeviceImageTy &Image,
+                                         const GlobalTy &HostGlobal,
+                                         bool Device2Host);
----------------
Clang format everything. Try `git clang-format HEAD~1`.


================
Comment at: openmp/libomptarget/plugins-nextgen/common/PluginInterface/PluginInterface.cpp:272-275
+      INFO(OMP_INFOTYPE_ALL, DeviceId,
+           "Unexpected host entry without address (size: %ld), abort!\n",
+           Entry->size);
+      return StatusCode::FAIL;
----------------
Stuff like this is why I think using LLVM's error would be good, e.g. we print the error at the entry to the plugin and return them like this.
```
return createStringError(llvm::inconvertibleErrorCode(),
                                          "Unexpected host entry without address (size: %ld), abort!\n", Entry->size);
```


================
Comment at: openmp/libomptarget/plugins-nextgen/common/PluginInterface/PluginInterface.cpp:376
+    }
+    LLVM_FALLTHROUGH;
+  case TARGET_ALLOC_HOST:
----------------
We don't need `LLVM_FALLTHROUGH` now that we're on C++17.


================
Comment at: openmp/libomptarget/plugins-nextgen/common/PluginInterface/PluginInterface.h:112
+    __tgt_target_table TTTablePtr;
+    std::map<void *, unsigned> EntryMap;
+    std::vector<__tgt_offload_entry> Entries;
----------------
Do we need these sorted? `std::map` is very slow compared to `std::unordered_map` or `llvm::DenseMap`. prefer the LLVM data structures whenever possible as they are faster.


================
Comment at: openmp/libomptarget/plugins-nextgen/common/PluginInterface/PluginInterface.h:423
+  /// deallocated by the allocator.
+  std::vector<DeviceImageTy *> LoadedImages;
+
----------------
`llvm::SmallVector` and below.


================
Comment at: openmp/libomptarget/plugins-nextgen/common/PluginInterface/PluginInterface.h:615-616
+
+  /// Get a string description from a status code.
+  static const char *getErrorStr(const StatusCode &SC);
+};
----------------
If we use LLVM's errors this is just `toString`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134396