[Openmp-commits] [PATCH] D104559: [OpenMP] Improve ref count debug messages

[Joel E. Denny via Phabricator via Openmp-commits]

jdenny added a comment.

In D104559#2827772 <https://reviews.llvm.org/D104559#2827772>, @jhuber6 wrote:

> Thanks for addressing this. Please update the documentation as well `./openmp/docs/design/Runtimes.rst`.

Thanks, I'll work on that.



================
Comment at: openmp/libomptarget/src/device.cpp:229
          (IsImplicit ? " (implicit)" : ""), DPxPTR(HstPtrBegin), DPxPTR(tp),
-         Size, (UpdateRefCount ? " updated" : ""),
-         HT.isRefCountInf() ? "INF" : std::to_string(HT.getRefCount()).c_str(),
+         Size, HT.refCountToStr().c_str(), RefCountAction,
          (HstPtrName) ? getNameFromMapping(HstPtrName).c_str() : "unknown");
----------------
jhuber6 wrote:
> 
I would prefer to determine the action and the record of the action at the same time instead of duplicating the logic.  I think that's easier to maintain.  However, this case is simple, so I can change it if you feel strongly.


================
Comment at: openmp/libomptarget/src/device.cpp:322
+         DPxPTR(HstPtrBegin), DPxPTR(tp), Size, HT.refCountToStr().c_str(),
+         RefCountAction);
     rc = (void *)tp;
----------------
jhuber6 wrote:
> Could probably do the same here, having nested ternary might look a little silly however.
In this more complicated case (and D104560 makes it even more complicated), I feel strongly that we should determine the action and the record of the action at the same time instead of duplicating the logic.  Is that ok with you?


================
Comment at: openmp/libomptarget/src/device.h:92
+
+  std::string refCountToStr() const {
+    return isRefCountInf() ? "INF" : std::to_string(getRefCount());
----------------
jhuber6 wrote:
> We'll also want to do this for dumping the device table, since anything with an infinite ref count will be printed at the end of the program.
Thanks, I'll change that.  Is it just `dumpTargetPointerMappings`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104559