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

[Kevin Sala Penadés via Phabricator via llvm-commits]

kevinsala added inline comments.


================
Comment at: openmp/libomptarget/include/Utilities.h:35
+/// from llvm::Error.
+class TgtError : public llvm::Error {
+public:
----------------
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.


================
Comment at: openmp/libomptarget/plugins-nextgen/common/PluginInterface/GlobalHandler.h:41
+  // holding a private copy of the name as a std::string
+  std::string Name;
+  uint32_t Size;
----------------
jhuber6 wrote:
> kevinsala wrote:
> > jhuber6 wrote:
> > > This should be able to be a `StringRef` as well.
> > Do you want to change it to `StringRef` for performance reasons (i.e., avoid the std::string's dynamic memory)?
> > 
> > Using `StringRef` here could be dangerous since we have no control on the string memory lifetime. This can be dangerous if a developer constructs a string on the stack, creates a `GlobalTy` variable, and passes that string as the name (e.g., as we do for the `exec_mode`). Thus, we would be adding an extra restriction on `GlobalTy`, where the user must guarantee that the string name has a longer lifetime than the `GlobalTy` object.
> > 
> > I would keep it as it is now, and if we see there is a performance penalty on using `std::string` here, we can change it.
> This is a huge patch so I can't really get a good view of the usage, but I figured that this will always point to a global inside the `omp_offloading_entires` section, which will guarantee we always have access to this memory as it's just a string constant in the binary itself. Does the new plugin interface add new constants not contained in the entry list?
There is an example of the usage in line 326 in `openmp/libomptarget/plugins-nextgen/cuda/src/rtl.cpp`. In that case, the global name is constructed from the kernel name + "_exec_mode", it's just a temporary string, and then it's safely copied to the `GlobalTy`'s `Name` member. We could move the string construction outside the `GlobalTy` constructor, and pass its `StringRef` to the ctor, but it seems dangerous from my point of view. It wouldn't be difficult to break this code at the slightest distraction.


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

https://reviews.llvm.org/D134396