[ORC][JITLink] Add Intel VTune support to JITLink #83957

hongyu-dev · 2024-03-05T04:08:15Z

This patch relands #81826

Note to reviewers: I split this pull requests in two commits. One for reland #81826 and the other one for fixing #83909. I'll squash those two into one once they are ready to be merged.

This patch adds two plugins: VTuneSupportPlugin.cpp and JITLoaderVTune.cpp. The testing is done in a manner similar to llvm-jitlistener. Currently, we only support the old version of Intel VTune API.

This patch relands llvm#81826 This patch adds two plugins: VTuneSupportPlugin.cpp and JITLoaderVTune.cpp. The testing is done in a manner similar to llvm-jitlistener. Currently, we only support the old version of Intel VTune API.

Fixed the cyclic library dependency: llvm#83909

hongyu-dev · 2024-03-05T04:09:00Z

@MaskRay @lhames Could you guys help me to review this? Thank you!

github-actions · 2024-03-05T04:10:56Z

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff 564b81db8541468ce296d3bdcacab05b9581297c e7e6d8f4a2e268b633e613a17a4c1d20868e3332 -- llvm/include/llvm/ExecutionEngine/Orc/Debugging/VTuneSupportPlugin.h llvm/include/llvm/ExecutionEngine/Orc/Shared/VTuneSharedStructs.h llvm/include/llvm/ExecutionEngine/Orc/TargetProcess/JITLoaderVTune.h llvm/lib/ExecutionEngine/Orc/Debugging/VTuneSupportPlugin.cpp llvm/lib/ExecutionEngine/Orc/TargetProcess/JITLoaderVTune.cpp llvm/tools/llvm-jitlink/llvm-jitlink.cpp

View the diff from clang-format here.

diff --git a/llvm/include/llvm/ExecutionEngine/Orc/TargetProcess/JITLoaderVTune.h b/llvm/include/llvm/ExecutionEngine/Orc/TargetProcess/JITLoaderVTune.h
index afb7df592f..52b9476947 100644
--- a/llvm/include/llvm/ExecutionEngine/Orc/TargetProcess/JITLoaderVTune.h
+++ b/llvm/include/llvm/ExecutionEngine/Orc/TargetProcess/JITLoaderVTune.h
@@ -27,5 +27,3 @@ extern "C" llvm::orc::shared::CWrapperFunctionResult
 llvm_orc_test_registerVTuneImpl(const char *Data, uint64_t Size);
 
 #endif // LLVM_EXECUTIONENGINE_ORC_TARGETPROCESS_JITLOADERVTUNE_H
-
-

MaskRay

I don't know the code, so I am just commenting on the code style...

MaskRay · 2024-03-05T05:03:04Z

llvm/include/llvm/ExecutionEngine/Orc/Debugging/VTuneSupportPlugin.h

+  ExecutorAddr RegisterVTuneImplAddr;
+  ExecutorAddr UnregisterVTuneImplAddr;
+  std::mutex PluginMutex;
+  uint64_t NextMethodID{0};


MaskRay · 2024-03-05T05:03:24Z

llvm/include/llvm/ExecutionEngine/Orc/Debugging/VTuneSupportPlugin.h

+  ExecutorAddr UnregisterVTuneImplAddr;
+  std::mutex PluginMutex;
+  uint64_t NextMethodID{0};
+  DenseMap<MaterializationResponsibility *, std::pair<uint64_t, uint64_t>>


include DenseMap for IWYU

MaskRay · 2024-03-05T05:03:51Z

llvm/include/llvm/ExecutionEngine/Orc/Shared/VTuneSharedStructs.h

+  VTuneStringTable Strings;
+};
+
+typedef std::vector<std::pair<uint64_t, uint64_t>> VTuneUnloadedMethodIDs;


nit: prefer using

MaskRay · 2024-03-05T05:04:35Z

llvm/lib/ExecutionEngine/Orc/Debugging/VTuneSupportPlugin.cpp

+using namespace llvm::orc;
+using namespace llvm::jitlink;
+
+namespace {


https://llvm.org/docs/CodingStandards.html#anonymous-namespaces

In .cpp, basically anonymous namespaces are only for struct, class, enum, etc.

MaskRay · 2024-03-05T05:04:53Z

llvm/lib/ExecutionEngine/Orc/TargetProcess/CMakeLists.txt

@@ -2,10 +2,18 @@ if( CMAKE_HOST_UNIX AND HAVE_LIBRT )
  set(rt_lib rt)
 endif()

+set(intel_jit_profiling )
+if( LLVM_USE_INTEL_JITEVENTS )
+set(intel_jit_profiling IntelJITProfiling)


MaskRay · 2024-03-05T05:05:47Z

llvm/lib/ExecutionEngine/Orc/TargetProcess/JITLoaderVTune.cpp

+using namespace llvm;
+using namespace llvm::orc;
+
+static std::unique_ptr<IntelJITEventsWrapper> Wrapper;


Can this global variable be placed into a context class?

Hi Fangrui, I fixed it into a context class, but I do not see the reason why we need a context class here because Wrapper is only used in JITLoaderVTune.cpp. Could you please elaborate more on it? Thank you!

The idea is to avoid a global state, more preferrable if the state can be shared between multiple threads.

Thank you! Fixed! Please let me know if there is anything else I need to change.

MaskRay · 2024-03-05T05:05:58Z

llvm/lib/ExecutionEngine/Orc/TargetProcess/JITLoaderVTune.cpp

+
+// For Testing: following code comes from llvm-jitlistener.cpp in llvm tools
+namespace {
+typedef std::vector<std::pair<std::string, unsigned int>> SourceLocations;


MaskRay · 2024-03-05T05:06:27Z

llvm/test/ExecutionEngine/JITLink/x86-64/ELF_vtune.s

@@ -0,0 +1,52 @@
+# REQUIRES: native && x86_64-linux && intel-jitevents
+
+# RUN: rm -rf %t && mkdir -p %t


remove -p since the file/directory is guaranteed to be absent

MaskRay · 2024-03-05T05:07:31Z

llvm/test/ExecutionEngine/JITLink/x86-64/ELF_vtune.s

+# REQUIRES: native && x86_64-linux && intel-jitevents
+
+# RUN: rm -rf %t && mkdir -p %t
+# RUN: llvm-mc -triple=x86_64-unknown-linux -position-independent \


remove -position-independent. Only mips/sparc use the option for some assembly design flaws.

MaskRay · 2024-03-05T05:08:45Z

llvm/include/llvm/ExecutionEngine/Orc/Debugging/VTuneSupportPlugin.h

+  uint64_t NextMethodID{0};
+  DenseMap<MaterializationResponsibility *, std::pair<uint64_t, uint64_t>>
+      PendingMethodIDs;
+  DenseMap<ResourceKey, std::vector<std::pair<uint64_t, uint64_t>>>


Consider replacing std::vector<X> with SmallVector<X, 0>, since SmallVector is smaller in size and is often more efficient. When used in another container like DenseMap , the advantage of SmallVector is larger.

Fixed it too!

MaskRay · 2024-03-05T05:09:51Z

bazel-6.3.2 build --config=generic_clang @llvm-project//llvm:llvm-jitlink passes, so the library layering seems good.

lhames

Minor comments aside this looks great. Thank you for resolving the layering issue!

lhames · 2024-03-07T16:40:01Z

llvm/include/llvm/ExecutionEngine/Orc/Debugging/VTuneSupportPlugin.h

+#ifndef LLVM_EXECUTIONENGINE_ORC_AMPLIFIERSUPPORTPLUGIN_H
+#define LLVM_EXECUTIONENGINE_ORC_AMPLIFIERSUPPORTPLUGIN_H


The include guards should match the filename -- LLVM_EXECUTIONENGINE_ORC_DEBUGGING_VTUNESUPPORT_H.

lhames · 2024-03-07T16:42:36Z

llvm/include/llvm/ExecutionEngine/Orc/Shared/VTuneSharedStructs.h

+} // end namespace orc
+} // end namespace llvm
+
+#endif


Missing newline.

llvm/lib/ExecutionEngine/Orc/TargetProcess/JITLoaderVTune.cpp

lhames

LGTM! Thanks @yugier!

hongyu-dev added 2 commits March 4, 2024 19:59

[ORC][JITLink] Add Intel VTune support to JITLink

b892ab4

This patch relands llvm#81826 This patch adds two plugins: VTuneSupportPlugin.cpp and JITLoaderVTune.cpp. The testing is done in a manner similar to llvm-jitlistener. Currently, we only support the old version of Intel VTune API.

[ORC][JITLink] Factor out common VTune data structures into OrcShared

d4d1ca9

Fixed the cyclic library dependency: llvm#83909

fixup! [ORC][JITLink] Add Intel VTune support to JITLink

45df827

MaskRay reviewed Mar 5, 2024

View reviewed changes

hongyu-dev added 2 commits March 4, 2024 22:35

Addressed reviewer's comments.

e3fca11

fixup! Addressed reviewer's comments.

0d188f8

lhames reviewed Mar 7, 2024

View reviewed changes

fixup! fixup! Addressed reviewer's comments.

e7e6d8f

lhames approved these changes Mar 7, 2024

View reviewed changes

lhames merged commit 00f4121 into llvm:main Mar 7, 2024

hongyu-dev deleted the VTuneSupportUpstream branch March 14, 2024 04:12

vchuravy mentioned this pull request Mar 22, 2024

Profiling support in OrcV2 / JITLink #30869

Open

giordano mentioned this pull request Jun 18, 2024

Improvements to JITLink, to ease transition JuliaLang/julia#54841

Merged

		@@ -0,0 +1,52 @@
		# REQUIRES: native && x86_64-linux && intel-jitevents

		# RUN: rm -rf %t && mkdir -p %t

		#ifndef LLVM_EXECUTIONENGINE_ORC_AMPLIFIERSUPPORTPLUGIN_H
		#define LLVM_EXECUTIONENGINE_ORC_AMPLIFIERSUPPORTPLUGIN_H

[ORC][JITLink] Add Intel VTune support to JITLink #83957

[ORC][JITLink] Add Intel VTune support to JITLink #83957

Uh oh!

Conversation

hongyu-dev commented Mar 5, 2024

Uh oh!

hongyu-dev commented Mar 5, 2024

Uh oh!

github-actions bot commented Mar 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MaskRay left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MaskRay commented Mar 5, 2024

Uh oh!

lhames left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lhames left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot commented Mar 5, 2024 •

edited

Loading