[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: openmp/libomptarget/include/Utilities.h:35
+/// from llvm::Error.
+class TgtError : public llvm::Error {
+public:
----------------
jhuber6 wrote:
> 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.
I'm removing the `TgtError` class and using `llvm::Error` directly. In the following case (it's just an example among others), the usual procedure should be calling `handleError(std::move(Err))` just after creating the temporary `Err` instance (not actually used)? Or is there another cleaner way?

```
  Error Err = Error::success();
  // handleError(std::move(Err));
                                                                                 
  // Transfer the data from the source to the destination.                       
  if (Device2Host)                                                               
    Err = Device.dataRetrieve(HostGlobal.getPtr(), DeviceGlobal.getPtr(),        
                              HostGlobal.getSize(), nullptr);                    
  else                                                                           
    Err = Device.dataSubmit(DeviceGlobal.getPtr(), HostGlobal.getPtr(),          
                            HostGlobal.getSize(), nullptr);                      
                                                                                 
  DP("%s %s %u bytes associated with global symbol '%s' %s the device "          
     "(%p -> %p).\n", (Err) ? "Failed to" : "Successfully",                      
     Device2Host ? "read" : "write", HostGlobal.getSize(),                       
     HostGlobal.getName().data(), Device2Host ? "from" : "to",                   
     DeviceGlobal.getPtr(), HostGlobal.getPtr());                                
                                                                                 
  return Err;
```


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

https://reviews.llvm.org/D134396