[Openmp-commits] [PATCH] D69423: [NFC] [libomptarget] Move option.h into target_impl.h, name a magic number

[Jon Chesterfield via Phabricator via Openmp-commits]

JonChesterfield marked 2 inline comments as done.
JonChesterfield added inline comments.


================
Comment at: openmp/libomptarget/deviceRTLs/nvptx/src/debug.h:133
 NOINLINE static void log(const char *fmt, Arguments... parameters) {
+  int threadIdxMask = WARPSIZE - 1;
   printf(fmt, (int)blockIdx.x, (int)threadIdx.x, (int)(threadIdx.x / WARPSIZE),
-         (int)(threadIdx.x & 0x1F), parameters...);
----------------
ABataev wrote:
> JonChesterfield wrote:
> > Note that this change from 0x1F to a function of warp size means the mask is 0x3F on 64 wide systems.
> Maybe it would be better to add 2 new functions, like getWarpId() and getThreadIdInWarp() or something like this and make them platform-dependent?
Like it, thanks.


================
Comment at: openmp/libomptarget/deviceRTLs/nvptx/src/target_impl.h:57-62
+#ifndef TRUE
+#define TRUE 1
+#endif
+#ifndef FALSE
+#define FALSE 0
+#endif
----------------
ABataev wrote:
> JonChesterfield wrote:
> > ABataev wrote:
> > > Is this a platform-dependent thing?
> > The inline/noinline macros are different for cuda and hip. Possibly also different for opencl, though I haven't checked.
> > 
> > True/false macros are things that happened to be in option.h. True is unused, false has a few uses related to debug printing. I'd like to remove those subsequently.
> My question was not about inine/noinline, just about true/false stuff. They do not look like the platform dependent stuff. If you say that you can remove them, just remove them, no need to move it to the platform-specific header.
Also good, done.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69423