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

[Johannes Doerfert via Phabricator via Openmp-commits]

jdoerfert 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:
> > > 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). 
I'm OK with using `auto Err = ` in such conditionals. The type is clear.


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

https://reviews.llvm.org/D134396