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

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

kevinsala marked an inline comment as done.
kevinsala added inline comments.


================
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;
----------------
jdoerfert wrote:
> jhuber6 wrote:
> > kevinsala wrote:
> > > jhuber6 wrote:
> > > > 
> > > Does `llvm::DenseMap` keep the references to the previously inserted elements valid when someone else is inserting a new element? I understood it invalidates them, but maybe I'm wrong. In case they are invalidated, we couldn't switch to `llvm::DenseMap` directly. I implemented this section thinking on multiple threads checking ELF object files, where some threads may be consulting an already inserted ELFObjectFile (without holding the map lock) while another potential thread may be inserting a new one (with the lock acquired).
> > You're right on the iterator invalidation. That guarantee is pat of what makes `std::unordered_map` slower than `llvm::DenseMap` in general. I'll leave it up to your discretion if you think that we need to be extra careful about invalidating iterators here. I was just guessing that this was part of the initialization code, which is generally thread safe since it should be called from the executable's constructors.
> In general I'm with @jhuber6 here. If we don't really think we need persistent iterators we should not add expectations. 
I've switched to `llvm::DenseMap` and removed the map lock.


================
Comment at: openmp/libomptarget/plugins-nextgen/common/PluginInterface/PluginInterface.cpp:176
+  if (EnableMM)
+    MemoryManager = new MemoryManagerTy(*this, ThresholdMM);
+
----------------
jhuber6 wrote:
> We should probably just use a `unique_ptr` here to avoid the explicit `new` and `delete`.
We need to destroy the memory manager in `GenericDeviceTy::deinit()` in a specific order with respect to other device components. The memory manager will try to deallocate its allocations and we need the device resources still alive to do that. I left a comment in line 182 explaining that issue.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134396