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

Joseph Huber via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Oct 13 17:46:30 PDT 2022


jhuber6 added a comment.

Looks okay in general. Please address some of the remaining comments or mark them as done.



================
Comment at: openmp/libomptarget/include/Utilities.h:35
+/// from llvm::Error.
+class TgtError : public llvm::Error {
+public:
----------------
kevinsala wrote:
> jhuber6 wrote:
> > Does this really add enough functionality to justify a new polymorphic type? All this seems to do is turn `toString` and `consumeError` into member functions rather than a free function.
> This is mainly for these two reasons:
> 1) `TgtError Err` creates a checked success. I didn't find an easy way to do it with `llvm::Error`: a) the default ctor is protected and b) `Error Err = Error::success()` is a success but it must be checked. Is there an easy way to achieve the same?
> 2) `consume`and `consumeString` is an attempt to hide some syntax that does't add much information to the code reader.
Needing to check successes is a design feature, generally you use `llvm::Error`.
```
if (Error E = canError())
  handleError(E);
```
I'm guessing this also skirts around the `[[nodiscard]]` that `llvm::Error` uses? If possible I'd like to retain those semantics. For a success, it only needs to be converted to `bool` to be checked. `consumeError` in general is a hack so I wouldn't encourage it if possible.


================
Comment at: openmp/libomptarget/include/Utilities.h:166
+template <typename GetterFunctor, typename SetterFunctor>
+inline void Envar<Ty>::init(llvm::StringRef Name, GetterFunctor Getter,
+                            SetterFunctor Setter) {
----------------
Given that this function can fail, I would prefer using LLVM's style for classes that can return errors https://llvm.org/docs/ProgrammersManual.html#fallible-constructors. Essentially, construction is done from a static function that calls a private constructor. I'm not sure how much more effort this will be. Ideally we should have each function return an `Error` or `Expected` and then finally handle / check that at the exported library functions.

Sorry if I'm being obsessive with this, one of my grievances with `libomptarget` is that it didn't follow the LLVM styles so I figure that now is a good chance to change that.


================
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",
----------------
jhuber6 wrote:
> This should be an error message or a debug message, not an info message. Same for all the uses below.
Again, these should be changed to an error or debug message. Info is for user diagnostics during runtime.


================
Comment at: openmp/libomptarget/plugins-nextgen/common/PluginInterface/PluginInterface.cpp:172-174
+  bool EnableMM;
+  size_t ThresholdMM;
+  std::tie(ThresholdMM, EnableMM) = MemoryManagerTy::getSizeThresholdFromEnv();
----------------
We should be able to use C++17's structured bindings.


================
Comment at: openmp/libomptarget/plugins-nextgen/common/PluginInterface/PluginInterface.cpp:176
+  if (EnableMM)
+    MemoryManager = new MemoryManagerTy(*this, ThresholdMM);
+
----------------
We should probably just use a `unique_ptr` here to avoid the explicit `new` and `delete`.


================
Comment at: openmp/libomptarget/plugins-nextgen/common/PluginInterface/PluginInterface.cpp:550
+  int32_t NumDevices = Plugin.getNumDevices();
+  for (int32_t DeviceId = 0; DeviceId < NumDevices; ++DeviceId) {
+    if (!Plugin.isImageCompatible(DeviceId, Info))
----------------
Nit. can remove a lot of these braces below.


================
Comment at: openmp/libomptarget/plugins-nextgen/cuda/src/rtl.cpp:245
+    // Query attributes to determine number of threads/block and blocks/grid.
+    Err = getDeviceAttr(CU_DEVICE_ATTRIBUTE_MAX_GRID_DIM_X,
+                        GridValues.GV_Max_Teams);
----------------
A lot of these can be a single `if (Err = ...` like above.


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

https://reviews.llvm.org/D134396



More information about the llvm-commits mailing list