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

[Jan-Patrick Lehr via Phabricator via Openmp-commits]

jplehr added inline comments.


================
Comment at: openmp/libomptarget/src/OmptCallback.cpp:63
+/// Get the current target region id
+static uint64_t getRegionId() { return OmptInterface.getTargetDataValue(); }
+
----------------
mhalk wrote:
> 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.
Ok, I re-read the spec. In the light of the spec the name `regionId` makes more sense.
Maybe a dumb question, but do we need this function, or is it better to return the `ompt_data_t` itself, so it is left to the caller what they want to do with it?
I mean, do we need / want / use this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127365