[PATCH] D146169: Non-debuginfo JITLink perf jitdump support

[Lang Hames via Phabricator via llvm-commits]

lhames added a comment.

This looks great -- thanks very much @pchintalapudi!

I love that you've gone for the full cross-process implementation -- that's amazing. Have you tested it in a cross-process setup with `llvm-jitlink-executor`? Is it really working out-of-process already?

I have a few requests, mostly cosmetic (though the licensing info should go in before this lands), but other than that LGTM.



================
Comment at: llvm/include/llvm/ExecutionEngine/Orc/PerfSupportPlugin.h:1
+//===--- PerfSupportPlugin.h -- Utils for perf support ---*- C++ -*-===//
+//
----------------
Should be padded out to 80 cols with dashes either side of the name/description. 


================
Comment at: llvm/include/llvm/ExecutionEngine/Orc/PerfSupportPlugin.h:39-44
+  Error notifyRemovingResources(JITDylib &JD, ResourceKey K) override {
+    return Error::success();
+  }
+
+  void notifyTransferringResources(JITDylib &JD, ResourceKey DstKey,
+                                   ResourceKey SrcKey) override {}
----------------
Do the perf JIT APIs support unloading code? If so these should have a `// TODO` to fill them in later. (I don't think they're needed for the initial commit).


================
Comment at: llvm/include/llvm/ExecutionEngine/Orc/PerfSupportPlugin.h:135-140
+  static size_t size(const PerfJITRecordPrefix &Val) {
+    auto tup = std::make_tuple(static_cast<uint32_t>(Val.Id), Val.TotalSize,
+                               Val.Timestamp);
+    return SPSSerializationTraits<SPSPerfJITRecordPrefix, ImplTupleType>::size(
+        tup);
+  }
----------------
In SimplePackedSerialization argument lists and tuples are guaranteed to have the same serialization, so there's a convenience method on the SPS tuple list that you can use here:
```
static size_t size(const PerfJITRecordPrefix &Val) {
  return SPSPerfJITRecordPrefix::AsArgList::size(
    static_cast<uint32_t>(Val.Id), Val.TotalSize, Val.Timestamp);
}
```


================
Comment at: llvm/include/llvm/ExecutionEngine/Orc/PerfSupportPlugin.h:142-152
+    ImplTupleType Tup;
+    if (SPSSerializationTraits<SPSPerfJITRecordPrefix,
+                               ImplTupleType>::deserialize(IB, Tup)) {
+      Val.Id = static_cast<PerfJITRecordType>(std::move(std::get<0>(Tup)));
+      Val.TotalSize = std::move(std::get<1>(Tup));
+      Val.Timestamp = std::move(std::get<2>(Tup));
+      return true;
----------------
Likewise here:
```
static bool deserialize(SPSInputBuffer &IB, PerfJITRecordPrefix &Val) {
    uint32_t Id;
    if (SPSPerfJITRecordPrefix::AsArgList::deserialize(IB, Id, Val.TotalSize, Val.Timestamp)) {
      Val.Id = Id;
      return true;
    }
    return false;
}
```



================
Comment at: llvm/include/llvm/ExecutionEngine/Orc/PerfSupportPlugin.h:153-157
+  static bool serialize(SPSOutputBuffer &OB, const PerfJITRecordPrefix &Val) {
+    return SPSSerializationTraits<SPSPerfJITRecordPrefix, ImplTupleType>::
+        serialize(OB, std::make_tuple(static_cast<uint32_t>(Val.Id),
+                                      Val.TotalSize, Val.Timestamp));
+  }
----------------
And here:
```
static bool serialize(SPSOutputBuffer &OB, const PerfJITRecordPrefix &Val) {
  return SPSPerfJITRecordPrefix:AsArgList::
        serialize(OB, static_cast<uint32_t>(Val.Id), Val.TotalSize, Val.Timestamp);
}
```



================
Comment at: llvm/include/llvm/ExecutionEngine/Orc/PerfSupportPlugin.h:165
+template <>
+class SPSSerializationTraits<SPSPerfJITCodeLoadRecord, PerfJITCodeLoadRecord> {
+public:
----------------
Same advice as above here -- the methods below can probably be shortened by using `SPSPerfJITCodeLoadRecord::AsArgList`.


================
Comment at: llvm/include/llvm/ExecutionEngine/Orc/PerfSupportPlugin.h:207
+template <>
+class SPSSerializationTraits<SPSPerfJITDebugEntry, PerfJITDebugEntry> {
+public:
----------------
More potential `AsArgList` simplifications here.


================
Comment at: llvm/include/llvm/ExecutionEngine/Orc/PerfSupportPlugin.h:242
+template <>
+class SPSSerializationTraits<SPSPerfJITDebugInfoRecord,
+                             PerfJITDebugInfoRecord> {
----------------
More potential AsArgList simplifications here.


================
Comment at: llvm/include/llvm/ExecutionEngine/Orc/PerfSupportPlugin.h:278
+template <>
+class SPSSerializationTraits<SPSPerfJITCodeUnwindingInfoRecord,
+                             PerfJITCodeUnwindingInfoRecord> {
----------------
More potential AsArgList simplifications here.


================
Comment at: llvm/include/llvm/ExecutionEngine/Orc/PerfSupportPlugin.h:324
+template <>
+class SPSSerializationTraits<SPSPerfJITRecordBatch, PerfJITRecordBatch> {
+public:
----------------
More potential AsArgList simplifications here.


================
Comment at: llvm/include/llvm/ExecutionEngine/Orc/TargetProcess/JITLoaderPerf.h:1
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
----------------
Looks like this is missing the file name / description line.
```
//===------- JITLoaderPerf.h - Register profiler objects ------*- C++ -*-===//
```


================
Comment at: llvm/lib/ExecutionEngine/Orc/PerfSupportPlugin.cpp:1
+#include "llvm/ExecutionEngine/Orc/PerfSupportPlugin.h"
+
----------------
Needs LLVM licensing header.


================
Comment at: llvm/lib/ExecutionEngine/Orc/PerfSupportPlugin.cpp:22
+
+std::string serialize(LinkGraph &G) {
+  std::string out;
----------------
There's already a `LinkGraph::dump(raw_ostream&)` method. Could that be used instead of this?

Alternatively if the former is dumping too verbosely could we clean it up? Or add this to `LinkGraph` as `dumpCompact`?


================
Comment at: llvm/lib/ExecutionEngine/Orc/PerfSupportPlugin.cpp:47-86
+std::string createX64EHFrameHeader(Section &EHFrame,
+                                   support::endianness endianness,
+                                   bool absolute) {
+  uint8_t Version = 1;
+  uint8_t EhFramePtrEnc = 0;
+  if (absolute) {
+    EhFramePtrEnc |= dwarf::DW_EH_PE_sdata8 | dwarf::DW_EH_PE_absptr;
----------------
This shoul


================
Comment at: llvm/lib/ExecutionEngine/Orc/PerfSupportPlugin.cpp:82-83
+  } else {
+    memcpy(HeaderContent.data() + 4, &EHFrameRelocation,
+           sizeof(EHFrameRelocation));
+  }
----------------
It looks like the header is a known size -- you might be better off using a BinaryStreamWriter here.


================
Comment at: llvm/lib/ExecutionEngine/Orc/PerfSupportPlugin.cpp:100-113
+static inline uint64_t perf_get_timestamp() {
+#ifdef __linux__
+  struct timespec ts;
+  int ret;
+
+  ret = clock_gettime(CLOCK_MONOTONIC, &ts);
+  if (ret)
----------------
This is being used to get the timestamp in the host process. Should we be timestamping the deserialized records on the executor side instead?


================
Comment at: llvm/lib/ExecutionEngine/Orc/TargetProcess/JITLoaderPerf.cpp:1
+#include "llvm/ExecutionEngine/Orc/TargetProcess/JITLoaderPerf.h"
+
----------------
Needs LLVM licensing header.

We should also have a file-level TODO to move this into the ORC runtime once we can.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146169