[OpenMP] Remove copy constructor of `RTLInfoTy`

[tianshilei1992]

Summary

Multiple RTLInfoTy objects are stored in a list AllRTLs. Since
RTLInfoTy contains a std::mutex, it is by default not a copyable object.
In order to support AllRTLs.push_back(...) which is currently used, a customized
copy constructor is provided. Every time we need to add a new data member into
RTLInfoTy, we should keep in mind not forgetting to add corresponding assignment
in the copy constructor. In fact, the only use of the copy constructor is to push
the object into the list, we can of course write it in a way that first emplace
a new object back, and then use the reference to the last element. In this way we
don't need the copy constructor anymore. If the element is invalid, we just need
to pop it, and that's what this patch does.

[tianshilei1992]

openmp/libomptarget/src/rtl.cpp

170–171

The exit logic has a minor issue. Will fix it.

[tianshilei1992]

Fixed the continue condition

[tianshilei1992]

Added a missing plugin interface

[JonChesterfield]

Delighted to see the copy ctor go. That's an easy place to forget to update. It should probably be explicitly = delete.

Does the move constructor do the right thing on a std::mutex? It'll be called by the vector resizing. Will want it = default after the copy ctor is marked delete iirc.

openmp/libomptarget/src/rtl.cpp

98

A plugin missing one of the required functions is an infrequent error.

I think we can drop all the ValidPlugin && statements to get simpler code that runs faster in the normal case, and does a bunch of unnecessary but harmless dlsym on the failing path.

[tianshilei1992]

Dropped ValidPlugin &&

[tianshilei1992]

std::mutex is non-copyable and non-movable. Any struct containing it will be marked non-copyable and non-movable implicitly. Actually I'm not sure how compiler handles this struct because if I explicitly add RTLInfoTy(RTLInfoTy &&) = default and RTLInfoTy(const RTLInfoTy &) = delete, the compiler complains "no matching constructor for initialization of 'RTLInfoTy'". However, everything seems all good if I don't do anything...

[tianshilei1992]

In D94361#2488663, @tianshilei1992 wrote:

std::mutex is non-copyable and non-movable. Any struct containing it will be marked non-copyable and non-movable implicitly. Actually I'm not sure how compiler handles this struct because if I explicitly add RTLInfoTy(RTLInfoTy &&) = default and RTLInfoTy(const RTLInfoTy &) = delete, the compiler complains "no matching constructor for initialization of 'RTLInfoTy'". However, everything seems all good if I don't do anything...

Well, stupid me. AllRTLs is a std::list...

[JonChesterfield]

LGTM, thanks!

openmp/libomptarget/src/rtl.h

#endif
 
   // Are there images associated with this RTL.
   bool isUsed = false;
 
   // Mutex for thread-safety when calling RTL interface functions.
   // It is easier to enforce thread-safety at the libomptarget level,
   // so that developers of new RTLs do not have to worry about it.
   std::mutex Mtx;
-
-  // The existence of the mutex above makes RTLInfoTy non-copyable.
-  // We need to provide a copy constructor explicitly.
-  RTLInfoTy() = default;
-
-  RTLInfoTy(const RTLInfoTy &r) {
-    Idx = r.Idx;
-    NumberOfDevices = r.NumberOfDevices;
-    LibraryHandler = r.LibraryHandler;
-#ifdef OMPTARGET_DEBUG
-    RTLName = r.RTLName;
-#endif
-    is_valid_binary = r.is_valid_binary;
-    is_data_exchangable = r.is_data_exchangable;
-    number_of_devices = r.number_of_devices;
-    init_device = r.init_device;
-    load_binary = r.load_binary;
-    data_alloc = r.data_alloc;
-    data_submit = r.data_submit;
-    data_submit_async = r.data_submit_async;
-    data_retrieve = r.data_retrieve;
-    data_retrieve_async = r.data_retrieve_async;
-    data_exchange = r.data_exchange;
-    data_exchange_async = r.data_exchange_async;
-    data_delete = r.data_delete;
-    run_region = r.run_region;
-    run_region_async = r.run_region_async;
-    run_team_region = r.run_team_region;
-    run_team_region_async = r.run_team_region_async;
-    init_requires = r.init_requires;
-    isUsed = r.isUsed;
-    synchronize = r.synchronize;
-  }
 };
 
 /// RTLs identified in the system.
 class RTLsTy {
 private:
   // Mutex-like object to guarantee thread-safety and unique initialization
   // (i.e. the library attempts to load the RTLs (plugins) only once).
   std::once_flag initFlag;

openmp/libomptarget/src/rtl.cpp

for (auto *Name : RTLNames) {
     if (!dynlib_handle) {
       // Library does not exist or cannot be found.
       DP("Unable to load library '%s': %s!\n", Name, dlerror());
       continue;
     }
 
     DP("Successfully loaded library '%s'!\n", Name);
 
-    // Retrieve the RTL information from the runtime library.
-    RTLInfoTy R;
+    AllRTLs.emplace_back();
 
-    R.LibraryHandler = dynlib_handle;
-    R.isUsed = false;
+    // Retrieve the RTL information from the runtime library.
+    RTLInfoTy &R = AllRTLs.back();
 
-#ifdef OMPTARGET_DEBUG
-    R.RTLName = Name;
-#endif
+    bool ValidPlugin = true;
 
     if (!(*((void **)&R.is_valid_binary) =
               dlsym(dynlib_handle, "__tgt_rtl_is_valid_binary")))
-      continue;
+      ValidPlugin = false;
     if (!(*((void **)&R.number_of_devices) =
               dlsym(dynlib_handle, "__tgt_rtl_number_of_devices")))
-      continue;
+      ValidPlugin = false;
     if (!(*((void **)&R.init_device) =
               dlsym(dynlib_handle, "__tgt_rtl_init_device")))
-      continue;
+      ValidPlugin = false;
     if (!(*((void **)&R.load_binary) =
               dlsym(dynlib_handle, "__tgt_rtl_load_binary")))
-      continue;
+      ValidPlugin = false;
     if (!(*((void **)&R.data_alloc) =
               dlsym(dynlib_handle, "__tgt_rtl_data_alloc")))
-      continue;
+      ValidPlugin = false;
     if (!(*((void **)&R.data_submit) =
               dlsym(dynlib_handle, "__tgt_rtl_data_submit")))
-      continue;
+      ValidPlugin = false;
     if (!(*((void **)&R.data_retrieve) =
               dlsym(dynlib_handle, "__tgt_rtl_data_retrieve")))
-      continue;
+      ValidPlugin = false;
     if (!(*((void **)&R.data_delete) =
               dlsym(dynlib_handle, "__tgt_rtl_data_delete")))
-      continue;
+      ValidPlugin = false;
     if (!(*((void **)&R.run_region) =
               dlsym(dynlib_handle, "__tgt_rtl_run_target_region")))
-      continue;
+      ValidPlugin = false;
     if (!(*((void **)&R.run_team_region) =
               dlsym(dynlib_handle, "__tgt_rtl_run_target_team_region")))
+      ValidPlugin = false;
+
+    // Invalid plugin
+    if (!ValidPlugin) {
+      DP("Invalid plugin as necessary interface is not found.\n");
+      AllRTLs.pop_back();
+      continue;
+    }
+
+    // No devices are supported by this RTL?
+    if (!(R.NumberOfDevices = R.number_of_devices())) {
+      // The RTL is invalid! Will pop the object from the RTLs list.
+      DP("No devices supported in this RTL\n");
+      AllRTLs.pop_back();
       continue;
+    }
+
+    R.LibraryHandler = dynlib_handle;
+
+#ifdef OMPTARGET_DEBUG
+    R.RTLName = Name;
+#endif
+
+    DP("Registering RTL %s supporting %d devices!\n", R.RTLName.c_str(),
+       R.NumberOfDevices);
 
     // Optional functions
     *((void **)&R.init_requires) =
         dlsym(dynlib_handle, "__tgt_rtl_init_requires");
     *((void **)&R.data_submit_async) =
         dlsym(dynlib_handle, "__tgt_rtl_data_submit_async");
     *((void **)&R.data_retrieve_async) =
         dlsym(dynlib_handle, "__tgt_rtl_data_retrieve_async");
     *((void **)&R.run_region_async) =
         dlsym(dynlib_handle, "__tgt_rtl_run_target_region_async");
     *((void **)&R.run_team_region_async) =
         dlsym(dynlib_handle, "__tgt_rtl_run_target_team_region_async");
     *((void **)&R.synchronize) = dlsym(dynlib_handle, "__tgt_rtl_synchronize");
     *((void **)&R.data_exchange) =
         dlsym(dynlib_handle, "__tgt_rtl_data_exchange");
     *((void **)&R.data_exchange_async) =
         dlsym(dynlib_handle, "__tgt_rtl_data_exchange_async");
     *((void **)&R.is_data_exchangable) =
         dlsym(dynlib_handle, "__tgt_rtl_is_data_exchangable");
-
-    // No devices are supported by this RTL?
-    if (!(R.NumberOfDevices = R.number_of_devices())) {
-      DP("No devices supported in this RTL\n");
-      continue;
-    }
-
-    DP("Registering RTL %s supporting %d devices!\n", R.RTLName.c_str(),
-       R.NumberOfDevices);
-
-    // The RTL is valid! Will save the information in the RTLs list.
-    AllRTLs.push_back(R);
   }
 
   DP("RTLs loaded!\n");
 
   return;
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 // Functionality for registering libs