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

Kevin Sala Penad├ęs via Phabricator via Openmp-commits openmp-commits at lists.llvm.org
Mon Dec 12 07:31:51 PST 2022

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?

  rG LLVM Github Monorepo



More information about the Openmp-commits mailing list