[Openmp-commits] [PATCH] D87165: [OpenMP] Begin Printing Information Dumps In Libomptarget and Plugins

[Johannes Doerfert via Phabricator via Openmp-commits]

jdoerfert added a comment.

In D87165#2257208 <https://reviews.llvm.org/D87165#2257208>, @ye-luo wrote:

> In D87165#2257164 <https://reviews.llvm.org/D87165#2257164>, @jhuber6 wrote:
>
>> Added additional comments. Should I add them to the doxygen notes at the top?
>
> @jdoerfert Any suggestions?
>
> I think the current documentation is insufficient. Adding/updating any documents for any of the macro or overall are helpful.
> Although other macros are not touched, it is still helpful to document them like those you added. You are probably the best person who knows how and when such macros should be used now.

I guess I'd prefer doxygen above the macro, as if they are functions. The reason is that I want us to make them functions at some point anyway.



================
Comment at: openmp/libomptarget/plugins/cuda/src/rtl.cpp:500
+         DeviceData[DeviceId].BlocksPerGrid,
+         DeviceData[DeviceId].ThreadsPerBlock, DeviceData[DeviceId].WarpSize);
 
----------------
Maybe: "Device %d supports up to % CUDA blocks and %d threads with a warp size of %d".
Also, why don't we make info behave like DP in debug mode and remove the DP macro?


================
Comment at: openmp/libomptarget/plugins/cuda/src/rtl.cpp:943
+    INFO("Launching kernel with %d blocks and %d threads\n", CudaBlocksPerGrid,
+         CudaThreadsPerBlock);
+
----------------
Can we please merge those two messages:
```
INFO("Launching kernel with %d blocks and %d threads in %s mode\n", CudaBlocksPerGrid,
         CudaThreadsPerBlock,(KernelInfo->ExecutionMode == SPMD)? "SPMD" : "Generic")
```


================
Comment at: openmp/libomptarget/src/interface.cpp:37
+              DPxPTR(HostTargetMap.TgtPtrBegin),
+              HostTargetMap.HstPtrEnd - HostTargetMap.HstPtrBegin);
+    }
----------------
Don't we have to add space here to match the 18 above?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87165