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

[Jon Chesterfield via Phabricator via Openmp-commits]

JonChesterfield accepted this revision.
JonChesterfield added a comment.
This revision is now accepted and ready to land.

Some minor comments inline from a scan through. I'm still in favour of landing this and iterating in trunk, keeping the current deviceRTL live (and use the current one by default for now) until we're pretty sure the new one works as well. If it proves to be long lived we might want to include some files into both of them to cut out the duplication+divergence, but for now I'm hoping that won't be warranted.



================
Comment at: openmp/libomptarget/DeviceRTL/include/Mapping.h:79
+
+/// Return the number of processing elements on the device.
+uint32_t getNumberOfProcessorElements();
----------------
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?


================
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
----------------
typo mookup?


================
Comment at: openmp/libomptarget/DeviceRTL/include/Types.h:15
+
+/// Base type declarations for freestanding mode
+///
----------------
freestanding has stdint.h, stddef.h, couple of other headers reliably available


================
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); }
----------------
Nice. Might want a _Static_assert that sizeof(unsigned)==4 and sizeof(long) == 8.


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


================
Comment at: openmp/libomptarget/DeviceRTL/src/Debug.cpp:27
+
+  __builtin_assume(cond);
+}
----------------
Does this change anything? The !cond above hopefully propagates without the assume


================
Comment at: openmp/libomptarget/DeviceRTL/src/Kernel.cpp:21
+
+extern "C" bool __kmpc_kernel_parallel(ParallelRegionFnTy *WorkFn);
+
----------------
this declaration in a header I think, safer to include said header than redeclare it


================
Comment at: openmp/libomptarget/DeviceRTL/src/Kernel.cpp:48
+    // returning to the caller.
+    if (!WorkFn)
+      return;
----------------
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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106803