[Openmp-commits] [PATCH] D127365: [OpenMP] [OMPT] [6/8] Added callback support for target data operations, target submit, and target regions.

[Michael Halkenhäuser via Phabricator via Openmp-commits]

mhalk added a comment.

Thanks for pointing out these nits, I've prepped changes for the latter two (`class` & uppercase spelling) and will wait for further feedback.



================
Comment at: openmp/libomptarget/src/OmptCallback.cpp:63
+/// Get the current target region id
+static uint64_t getRegionId() { return OmptInterface.getTargetDataValue(); }
+
----------------
jplehr wrote:
> This naming seems inconsistent, doesn't it?
> Why is it called `TargetDataValue` in the `OmptInterface` but the function to access it `getRegionId`.
Yes, that is absolutely right.
Thanks for bringing this up.

Background:
According to the spec the underlying data type `ompt_data_t` is supposed to represent: "[...] data that is reserved for tool use and that is related to a thread or to a parallel or task region."

My observation so far is, that this is only used for (task / target) "regions".
>From the use-case POV, maybe `get[Target]RegionDataValue` (`get[Target]RegionDataPtr`)?

But, "correctly", you'd call it `getToolDataValue` (`getToolDataPtr`) I guess -- while rather generic, that'd be my pick.

If anyone has a preference / other suggestion, I'm happy to oblige.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127365