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

[Johannes Doerfert via Phabricator via Openmp-commits]

jdoerfert marked 8 inline comments as done.
jdoerfert added a comment.

I address things locally before I push, I'll wait a little longer but want to get this into 13.



================
Comment at: openmp/libomptarget/DeviceRTL/include/Mapping.h:79
+
+/// Return the number of processing elements on the device.
+uint32_t getNumberOfProcessorElements();
----------------
JonChesterfield wrote:
> Does this mean number of streaming multiprocessor / compute unit? A little surprised we need to know that.
> 
> Can't see a call site, can we delete this function?
Yes it is. Call site:
`int omp_get_num_procs(void) { return mapping::getNumberOfProcessorElements(); } `



================
Comment at: openmp/libomptarget/DeviceRTL/include/State.h:98
+
+/// A mookup class without actual state used to provide
+/// a nice interface to lookup and update ICV values
----------------
JonChesterfield wrote:
> typo mookup?
on purpose to check if people pay attention ;) 


================
Comment at: openmp/libomptarget/DeviceRTL/include/Types.h:15
+
+/// Base type declarations for freestanding mode
+///
----------------
JonChesterfield wrote:
> freestanding has stdint.h, stddef.h, couple of other headers reliably available
I'll leave that for a follow up cleanup. Static assert will catch problems for now.


================
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:
> 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?


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


================
Comment at: openmp/libomptarget/DeviceRTL/src/Debug.cpp:27
+
+  __builtin_assume(cond);
+}
----------------
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.


================
Comment at: openmp/libomptarget/DeviceRTL/src/Kernel.cpp:48
+    // returning to the caller.
+    if (!WorkFn)
+      return;
----------------
JonChesterfield wrote:
> existing hazard. a shame that this construct blocks inlining the work function, even when the only two values stored to the variable are 0 and a given work function
it does not as we build custom state machines anyway. This is the O0 fallback only, functionally correct.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106803