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

[Johannes Doerfert via Phabricator via Openmp-commits]

jdoerfert added a comment.

I have a lot of style/organization comments but nothing major. If nobody has a conceptual problem we merge this in once the code is reorganized and then take it from there.



================
Comment at: openmp/libomptarget/plugins-nextgen/common/PluginInterface/PluginInterface.cpp:58
+  }
+
   return initImpl(GenericDevice, Image);
----------------
Let's put all these code snippets into a namespace or struct as static functions.
Then they are not "in the way" of the main logic.
```
if (isRecording())
  recording::saveImage(Image, Name);
```
Since there is state associated with recording, e.g., the memory pointers, we might even want a struct
```
if (RecordReplay.isRecording())
  RecordReplay.saveImage(Image, Name);
```


================
Comment at: openmp/libomptarget/plugins-nextgen/common/PluginInterface/PluginInterface.cpp:213
+    MemorySize = 0;
+  }
+
----------------
I'd start at more than 8Gb, and I also would like to catch 512Mb and such. Maybe we want a list of "common/known" memory sizes? 64, 32, 24, 20, 16, 10, 8, 6, 4, 2, 1, .5, .25, something like that? (Not that I know how much memory GPUs have...)


================
Comment at: openmp/libomptarget/plugins-nextgen/common/PluginInterface/PluginInterface.cpp:392
+    return Alloc;
+  }
+
----------------
I imagine this needs to be integrated into the memory manager at some point to support free.
The manager keeps free lists and such and we can let it keep one list for all sizes bigger than what it tracks now.
When we record we go though all the allocated memory and dump it in a "sparse format" basically.
Replay then the opposite.
All that said, this can be a TODO for now explaining it.


================
Comment at: openmp/libomptarget/plugins-nextgen/common/PluginInterface/PluginInterface.cpp:543
+    OS.close();
+  }
+
----------------
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.


================
Comment at: openmp/libomptarget/src/interface.cpp:258
 
+EXTERN int __tgt_target_kernel_replay(ident_t *Loc, int64_t DeviceId,
+                                      void *HostPtr, void *DeviceMemory,
----------------
Documentation.


================
Comment at: openmp/libomptarget/src/omptarget.cpp:1619
+                               NumTeams, ThreadLimit, LoopTripCount, AsyncInfo);
+  }
+
----------------
No need for the block.
Above, the comments should be full sentences and actually describe more than the immediate code following.


================
Comment at: openmp/libomptarget/tools/kernelreplay/llvm-omp-kernel-replay.cpp:54
+  if (!KernelInfoMB)
+    report_fatal_error("Error create MB for kernel info");
+  Expected<json::Value> JsonKernelInfo =
----------------
This is potentially user facing, MB and such doesn't mean much.


================
Comment at: openmp/libomptarget/tools/kernelreplay/llvm-omp-kernel-replay.cpp:66
+  unsigned NumThreads =
+      (NumThreadsOpt > 0 ? NumThreadsOpt : NumThreadsJson.value());
+  auto LoopTripCount =
----------------
Add a TODO that we should warn if the value was set before and is now changed.


================
Comment at: openmp/libomptarget/tools/kernelreplay/llvm-omp-kernel-replay.cpp:109
+
+  setenv("LIBOMPTARGET_REPLAY", "1", 1);
+
----------------
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.


================
Comment at: openmp/libomptarget/tools/kernelreplay/llvm-omp-kernel-replay.cpp:123
+      TgtArgOffsets.data(), NumArgs.value(), NumTeams, NumThreads,
+      LoopTripCount.value());
+
----------------
DeviceId needs to be stored and read I assume. We might also want a cmd line option.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138931