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

[Kevin Sala Penadés via Phabricator via Openmp-commits]

kevinsala added inline comments.


================
Comment at: openmp/libomptarget/plugins-nextgen/common/PluginInterface/PluginInterface.cpp:203
+    for (size_t Try = MAX_MEMORY_ALLOCATION; Try > 0; Try -= STEP) {
+      MemoryStart = allocate(Try, /* HstPtr */ nullptr, TARGET_ALLOC_DEFAULT);
+      if (MemoryStart)
----------------
Are we deallocating this memory buffer? If that's handled by the memory manager, this memory will automatically be freed at manager's finalization. But anyway, I think it's a good practice if we delete it during device deinitialization.


================
Comment at: openmp/libomptarget/plugins-nextgen/common/PluginInterface/PluginInterface.cpp:390
+    MemoryPtr = (char *)MemoryPtr + AlignedSize;
+    MemorySize += AlignedSize;
+    return Alloc;
----------------
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?



================
Comment at: openmp/libomptarget/plugins-nextgen/common/PluginInterface/PluginInterface.h:417
+  BoolEnvar RecordKernel;
+  BoolEnvar ReplayKernel;
+
----------------
Could we rename the envars to `OMPX_RecordKernel` and `OMPX_ReplayKernel`, as the rest of envars?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138931