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

[Joseph Huber via Phabricator via Openmp-commits]

jhuber6 added inline comments.


================
Comment at: openmp/libomptarget/include/Utilities.h:35
+/// from llvm::Error.
+class TgtError : public llvm::Error {
+public:
----------------
kevinsala wrote:
> 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;
> ```
I would probably write this more like the following:
```
// Transfer the data from the source to the destination.                       
if (Device2Host)                                                               
  if (llvm::Error Err = Device.dataRetrieve(HostGlobal.getPtr(), DeviceGlobal.getPtr(),        
                                                                     HostGlobal.getSize(), nullptr))
    return Err;                   
else if (llvm::Error Err = Device.dataSubmit(DeviceGlobal.getPtr(), HostGlobal.getPtr(),          
                                                                         HostGlobal.getSize(), nullptr);                      
    return Err;                                                                                 
DP("%s %s %u bytes associated with global symbol '%s' %s the device "          
   "(%p -> %p).\n", "Successfully",                      
   Device2Host ? "read" : "write", HostGlobal.getSize(),                       
   HostGlobal.getName().data(), Device2Host ? "from" : "to",                   
   DeviceGlobal.getPtr(), HostGlobal.getPtr());                                
                                                                               
return Error::Success();
```

Generally, `llvm::Error` is used to indicate an unrecoverable problem. When we encounter one we immediately return and then the top of the stack will handle and report the error. For `libomptarget` plugins this will probably involve the `__tgt_rtl_` functions printing the contents of the message via `toString`.

Occasionally you can ignore errors, but that should be reserved for cases where you know that you can continue without it (We do this in a few places). 


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

https://reviews.llvm.org/D134396