[PATCH] D146411: Basic VTune support for JITLink

[Lang Hames via Phabricator via llvm-commits]

lhames added inline comments.


================
Comment at: llvm/include/llvm/ExecutionEngine/Orc/VTuneSupportPlugin.h:13-14
+
+#ifndef LLVM_EXECUTIONENGINE_ORC_AMPLIFIERSUPPORTPLUGIN_H
+#define LLVM_EXECUTIONENGINE_ORC_AMPLIFIERSUPPORTPLUGIN_H
+
----------------
Include guards should match the file name.


================
Comment at: llvm/lib/ExecutionEngine/Orc/VTuneSupportPlugin.cpp:86
+             RegisterVTuneImplAddr, Batch)),
+         {}});
+    return Error::success();
----------------
It looks like you're deregistering in `notifyRemovingResources` below, but you sould add the deregistration call as a deallocation action here (replacing the no-op `(}`) if possible. Deallocation actions reduce IPC traffic, and they can be run during executor teardown even if the connection to the JIT has dropped out. It also saves you the hassle of tracking resources in your plugin (it becomes the memory manager's problem).


================
Comment at: llvm/lib/ExecutionEngine/Orc/VTuneSupportPlugin.cpp:149-161
+  auto &ES = EPC.getExecutionSession();
+  auto RegisterImplName = ES.intern(RegisterVTuneImplName);
+  auto UnregisterImplName = ES.intern(UnregisterVTuneImplName);
+  SymbolLookupSet SLS{RegisterImplName, UnregisterImplName};
+  auto Res = ES.lookup(makeJITDylibSearchOrder({&JD}), std::move(SLS));
+  if (!Res)
+    return Res.takeError();
----------------
You can use the `lookupAndRecordAddrs` utility in `llvm/ExecutionEngine/Orc/LookupAndRecordAddrs.h` to simplify this to:

```
  auto &ES = EPC.getExecutionSession();
  ExecutorAddr RegisterImplAddr;
  ExecutorAddr UnregisterImplAddr;
  if (auto Err = lookupAndRecordAddrs(
        ES, LookupKind::Static, makeJITDylibSearchOrder(&PJD),
        {
          {ES.intern(RegisterVTuneImplName), &RegisterImplAddr},
          {ES.intern(UnregisterVTuneImplName), &UnregisterImplAddr},
        }))
    return Err;
  return std::make_unique<VTuneSupportPlugin>(
      EPC, RegisterImplAddr, UnregisterImplAddr, EmitDebugInfo);
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146411