[Openmp-commits] [PATCH] D138931: [OpenMP] Support kernel record and replay

[Giorgis Georgakoudis via Phabricator via Openmp-commits]

ggeorgakoudis marked an inline comment as done.
ggeorgakoudis added inline comments.


================
Comment at: openmp/libomptarget/plugins-nextgen/common/PluginInterface/PluginInterface.cpp:390
+    MemoryPtr = (char *)MemoryPtr + AlignedSize;
+    MemorySize += AlignedSize;
+    return Alloc;
----------------
kevinsala wrote:
> I've several doubts/questions because I'm surely missing some context. I'm sorry if these comments do not apply in record/replay mode:
> 
> 1) Can multiple threads allocate data concurrently on record/replay mode? This part seems not thread-safe.
> 2) We are ignoring the `Kind` parameter here and always returning `TARGET_ALLOC_DEFAULT` data (pre-allocated at initialization). That's not a problem? Isn't possible to request other types of memory (e.g., host memory)?
> 3) This part does not handle the case where we exceed the maximum pre-allocated memory, right?
> 
Thank you for feedback, always welcome!

1. Good thinking, added a lock guard when allocating memory and a TODO
2. Yes, the patch is about the "common case". Added a TODO
3. No, it does not. Maybe add a TODO though we try to allocate the whole of device memory


================
Comment at: openmp/libomptarget/plugins-nextgen/common/PluginInterface/PluginInterface.cpp:543
+    OS.close();
+  }
+
----------------
jdoerfert wrote:
> These certainly need to go into some sort of helper functions.
> A TODO for filtering is also required.
> I'd personally rename "golden" to "original.output" or something descriptive, characters are free I guess.
> No comments after "else". If anything, make it an assertion.
I'm not sure what you mean by filtering


================
Comment at: openmp/libomptarget/tools/kernelreplay/llvm-omp-kernel-replay.cpp:109
+
+  setenv("LIBOMPTARGET_REPLAY", "1", 1);
+
----------------
jdoerfert wrote:
> I'm not 100% sold on the env var approach but I don't see a nice alternative that is not invasive.
> That said, we should use a bitset and communicate if we need a ".replay" output for verification or not.
> User might also want to choose a ".replay" output w/o verification via an option, but for perf testing, getting counters, etc. we should just skip that part.
I couldn't think of a better alternative either. I'm using `setenv` which is unix specific, I couldn't find a portable interface in LLVM.

I ended up adding an env var for saving the kernel device memory output, thinking that it may be the case that we don't want to save the output even when recording.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138931