[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 a comment.

Added some nits.
Any more comments @jdoerfert @dreachem @dhruvachak before accepting this?



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


================
Comment at: openmp/libomptarget/src/OmptInterface.h:43
+/// Used to maintain execution state for this thread
+struct Interface {
+public:
----------------
This should probably be a class according to the coding standards (sorry if I'm only pointing to this now).
https://llvm.org/docs/CodingStandards.html#use-of-class-and-struct-keywords


================
Comment at: openmp/libomptarget/src/OmptInterface.h:112
+  /// Setters for target region and target operations correlation ids
+  void setHostOpId(ompt_id_t id) { HostOpId = id; }
+  void setTargetDataValue(uint64_t val) { TargetData.value = val; }
----------------
Parameter names here and below spelling starting w/ uppercase?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127365