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

Mon Oct 24 23:35:23 PDT 2022

kevinsala marked an inline comment as done.
kevinsala added inline comments.

================
Comment at: openmp/libomptarget/plugins-nextgen/common/PluginInterface/PluginInterface.h:162
+               ptrdiff_t *ArgOffsets, int32_t NumArgs, int32_t NumTeamsClause,
+               int32_t ThreadLimitClause, int32_t LoopTripCount,
+               AsyncInfoWrapperTy &AsyncInfoWrapper) const;
----------------
jhuber6 wrote:
> kevinsala wrote:
> > jhuber6 wrote:
> > > Why do we use `int32_t` for the loop trip count when the interface uses `uint64_t`? I think it's totally possible someone would have a trip-count > 3 billion or so.
> > The loop trip count can decide the number of blocks/groups launched by a kernel. In CUDA and HSA, these parameters are `unsigned int`and `uint32_t`, respectively, and they are actually limited by the corresponding max blocks/groups properties of the device. So, although the program can generate a larger loop trip count, we will have to adapt the value at some point before the kernel launch. In this patch, we adapt the loop trip count to be the minimum between the original count and the `int32_t`'s maximum. The original plugins just cast the original `uint64_t` value to `unsigned int`/`int`.
> > 
> > Maybe we could continue using the method of this patch but emitting a debug message to notify the user that the loop trip count was changed.
> I think it would be better to do the narrowing in the implementation rather than the interface. That way we don't need to do this cast in every plugin and can have a single comment explaining why.
I agree it's better to use `uint64_t` directly for the loop trip count. I fixed that in the last version. I also changed some other fields to be unsigned (num threads, dynamic memory size, etc).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134396