[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 inline comments.


================
Comment at: openmp/libomptarget/src/OmptCallback.cpp:63
+/// Get the current target region id
+static uint64_t getRegionId() { return OmptInterface.getTargetDataValue(); }
+
----------------
jplehr wrote:
> 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?
Now, at a closer look, after all the refactoring and moving we can simply remove `getRegionId`.
AFAIK and as discussed offline, `getRegionId` would have no other utility because of its visbility.

Its uses will be replaced with direct access to the corresponding `TargetData.value` within "non-EMI" callback functions.
The EMI callbacks already use the `ompt_data_t TargetData` field directly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127365