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

[Jon Chesterfield via Phabricator via Openmp-commits]

JonChesterfield 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); }
----------------
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.


================
Comment at: openmp/libomptarget/DeviceRTL/src/Configuration.cpp:22
+  int32_t DebugLevel;
+  uint32_t NumDevices;
+};
----------------
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.


================
Comment at: openmp/libomptarget/DeviceRTL/src/Debug.cpp:27
+
+  __builtin_assume(cond);
+}
----------------
jdoerfert wrote:
> JonChesterfield wrote:
> > Does this change anything? The !cond above hopefully propagates without the assume
> It translates flow into value information, LLVM can't always do that by itself for now. Also, if we are in non-debug mode the entire conditional is just deleted, which is what we want, so cond would not provide information without the assume.
Ah, shame. You're right, `if (!cond && false)` can't fold into builtin_assume(cond).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106803