[Openmp-commits] [PATCH] D106803: [OpenMP] Prototype opt-in new GPU device RTL

[Johannes Doerfert via Phabricator via Openmp-commits]

jdoerfert marked 10 inline comments as done.
jdoerfert added inline comments.


================
Comment at: openmp/libomptarget/DeviceRTL/include/Utils.h:42
+
+/// Return the first bit set in \p V.
+inline uint32_t ffs(uint32_t V) { return __builtin_ffs(V); }
----------------
JonChesterfield wrote:
> jdoerfert wrote:
> > JonChesterfield wrote:
> > > Nice. Might want a _Static_assert that sizeof(unsigned)==4 and sizeof(long) == 8.
> > I tried to avoid non-sized types, did I miss any?
> It's the suffix on the builtin. ffs vs ffsl vs ffsll. I was burned by sizeof(long)==4 in the past so now compulsively put the static assert next to builtin functions that pass a uintxx_t to a function named based on normal/long/longlong. I hope windows builds of device code don't assume sizeof(long)==4 but wouldn't be totally shocked if they did.
I see now. Will do that.


================
Comment at: openmp/libomptarget/DeviceRTL/src/Configuration.cpp:22
+  int32_t DebugLevel;
+  uint32_t NumDevices;
+};
----------------
JonChesterfield wrote:
> jdoerfert wrote:
> > JonChesterfield wrote:
> > > number devices is an int32_t in some places but uint32_t here. I'd like it to be uint32_t everywhere, and if we're using -1 to indicate an error, to stop doing that
> > uint32 everywhere now.
> Not in the host plugins, so I assume not in libomptarget. Good to hear it's consistently unsigned in the device code.
yeah, to be fair it's not even hooked up to the host right now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106803