This is an archive of the discontinued LLVM Phabricator instance.

Profiling the code generated by MCJIT engine using Intel VTune profiler
ClosedPublic

Authored by ekovanov on Aug 24 2020, 2:21 AM.

Details

Summary

Problem:
LLVM already has a feature to profile the JIT-compiled code with VTune. This is done using Intel JIT Profiling API (https://github.com/intel/ittapi). Function information is captured by VTune as soon as the function is JIT-compiled. We tried to use the same approach to report the function information generated by the MCJIT engine – read parsing the debug information for in-memory ELF module and report it using JIT API. As the results, we figured out that it did not work properly for the following cases: inline functions, the functions located in multiple source files, the functions having several bodies (address ranges).

Solution:
To overcome limitations described above, we have introduced new APIs as a part of Intel ITT APIs to report the entire in-memory ELF module to be further processed as regular ELF binaries with debug information.

This patch

  1. Switches LLVM to open source version of Intel ITT/JIT APIs ((https://github.com/intel/ittapi) to keep it always up to date
  2. Adds support of profiling the code generated by MCJIT engine using Intel VTune profiler

Another separate patch will get rid of obsolete Intel ITT APIs stuff, having LLVM already switched to https://github.com/intel/ittapi

llvm/lib/ExecutionEngine/IntelJITEvents

ittnotify_config.h
ittnotify_types.h
jitprofiling.c
jitprofiling.h

Diff Detail

Event Timeline

ekovanov created this revision.Aug 24 2020, 2:21 AM
ekovanov requested review of this revision.Aug 24 2020, 2:21 AM

I'm not familiar with the profiling listeners. Out of interest: Is the backwards compatibility mode necessary? Is there a plan to deprecate it in the future?

ekovanov updated this revision to Diff 287750.Aug 25 2020, 12:34 PM

corrected indents that were reported by pre-merge checks

I'm not familiar with the profiling listeners. Out of interest: Is the backwards compatibility mode necessary? Is there a plan to deprecate it in the future?

@lhames yes, we plan to deprecate it in the future.
For now, by using backwards compatibility, we allow users to switch to the old flow in case of problems with the new one.

lhames accepted this revision.Aug 29 2020, 1:36 PM

@lhames yes, we plan to deprecate it in the future.
For now, by using backwards compatibility, we allow users to switch to the old flow in case of problems with the new one.

Sounds good to me. Thanks Elena!

This revision is now accepted and ready to land.Aug 29 2020, 1:36 PM
ekovanov updated this revision to Diff 289381.Sep 2 2020, 2:56 AM

updated diff with remarks for pre-merge checks

ekovanov updated this revision to Diff 289534.Sep 2 2020, 12:17 PM

corrections for pre-merge checks

Hi @lhames @andrew.w.kaylor

Looking at "Build Status" for the last diff, I can see 29 tests failed (windows) and 41 tests failed (linux).
do you think that tests failures are related to my commit?
Thank you in advance!

ekovanov updated this revision to Diff 290823.Sep 9 2020, 1:56 PM

added submodule update command

lhames added a comment.Oct 6 2020, 8:09 PM

Hi @lhames @andrew.w.kaylor

Looking at "Build Status" for the last diff, I can see 29 tests failed (windows) and 41 tests failed (linux).
do you think that tests failures are related to my commit?
Thank you in advance!

Hi Elena,

I can't be sure from the error messages -- I don't know the relevant profiling or LLDB APIs well enough. Are you able to reproduce any of them when you build locally?

ekovanov added a subscriber: craig.topper.EditedOct 7 2020, 11:11 AM

Hi @lhames @andrew.w.kaylor

Looking at "Build Status" for the last diff, I can see 29 tests failed (windows) and 41 tests failed (linux).
do you think that tests failures are related to my commit?
Thank you in advance!

Hi Elena,

I can't be sure from the error messages -- I don't know the relevant profiling or LLDB APIs well enough. Are you able to reproduce any of them when you build locally?

Hi @lhames !

I tried to run failed tests with applied diff on my system and tests have passed (master from 9th September was used).
But it constantly fails on Phabricator.
Also, I believe that most of my changes are related to -DLLVM_USE_INTEL_JITEVENTS=ON compile option, that does not participate in build process on Phabricator.

Hi @clayborg !

There are several failed tests for both Linux and Windows in "Build Status" section of this review.
The vast majority of failed tests is related to lldb.

But I wouldn’t expect my commit to have any effect unless -DLLVM_USE_INTEL_JITEVENTS=ON is used.
(the option -DLLVM_USE_INTEL_JITEVENTS=ON does not participate in build process on Phabricator)

I tried to run failed tests with applied diff on my system and tests have passed. (master from 9th September)

Do you think that tests failures are related to my commit?

Number of failed tests is unstable(multiple revisions of this patch), but there are consistently repeating items:

40 tests on Linux:

lldb-api :: commands/expression/import-std-module/basic/TestImportStdModule.py
lldb-api :: commands/expression/import-std-module/conflicts/TestStdModuleWithConflicts.py
lldb-api :: commands/expression/import-std-module/deque-basic/TestDequeFromStdModule.py
lldb-api :: commands/expression/import-std-module/deque-dbg-info-content/TestDbgInfoContentDequeFromStdModule.py
lldb-api :: commands/expression/import-std-module/forward_list-dbg-info-content/TestDbgInfoContentForwardListFromStdModule.py
lldb-api :: commands/expression/import-std-module/forward_list/TestForwardListFromStdModule.py
lldb-api :: commands/expression/import-std-module/list-dbg-info-content/TestDbgInfoContentListFromStdModule.py
lldb-api :: commands/expression/import-std-module/list/TestListFromStdModule.py
lldb-api :: commands/expression/import-std-module/no-std-module/TestMissingStdModule.py
lldb-api :: commands/expression/import-std-module/queue/TestQueueFromStdModule.py
lldb-api :: commands/expression/import-std-module/shared_ptr-dbg-info-content/TestSharedPtrDbgInfoContentFromStdModule.py
lldb-api :: commands/expression/import-std-module/shared_ptr/TestSharedPtrFromStdModule.py
lldb-api :: commands/expression/import-std-module/stack/TestStackFromStdModule.py
lldb-api :: commands/expression/import-std-module/unique_ptr-dbg-info-content/TestUniquePtrDbgInfoContent.py
lldb-api :: commands/expression/import-std-module/unique_ptr/TestUniquePtrFromStdModule.py
lldb-api :: commands/expression/import-std-module/vector-bool/TestVectorBoolFromStdModule.py
lldb-api :: commands/expression/import-std-module/vector-dbg-info-content/TestDbgInfoContentVectorFromStdModule.py
lldb-api :: commands/expression/import-std-module/vector-of-vectors/TestVectorOfVectorsFromStdModule.py
lldb-api :: commands/expression/import-std-module/vector/TestVectorFromStdModule.py
lldb-api :: commands/expression/import-std-module/weak_ptr-dbg-info-content/TestDbgInfoContentWeakPtrFromStdModule.py
lldb-api :: commands/expression/import-std-module/weak_ptr/TestWeakPtrFromStdModule.py
lldb-api :: functionalities/data-formatter/data-formatter-stl/libcxx/bitset/TestDataFormatterLibcxxBitset.py
lldb-api :: functionalities/data-formatter/data-formatter-stl/libcxx/forward_list/TestDataFormatterLibcxxForwardList.py
lldb-api :: functionalities/data-formatter/data-formatter-stl/libcxx/function/TestLibCxxFunction.py
lldb-api :: functionalities/data-formatter/data-formatter-stl/libcxx/iterator/TestDataFormatterLibccIterator.py
lldb-api :: functionalities/data-formatter/data-formatter-stl/libcxx/list/loop/TestDataFormatterLibcxxListLoop.py
lldb-api :: functionalities/data-formatter/data-formatter-stl/libcxx/map/TestDataFormatterLibccMap.py
lldb-api :: functionalities/data-formatter/data-formatter-stl/libcxx/multimap/TestDataFormatterLibccMultiMap.py
lldb-api :: functionalities/data-formatter/data-formatter-stl/libcxx/multiset/TestDataFormatterLibcxxMultiSet.py
lldb-api :: functionalities/data-formatter/data-formatter-stl/libcxx/optional/TestDataFormatterLibcxxOptional.py
lldb-api :: functionalities/data-formatter/data-formatter-stl/libcxx/queue/TestDataFormatterLibcxxQueue.py
lldb-api :: functionalities/data-formatter/data-formatter-stl/libcxx/set/TestDataFormatterLibcxxSet.py
lldb-api :: functionalities/data-formatter/data-formatter-stl/libcxx/string/TestDataFormatterLibcxxString.py
lldb-api :: functionalities/data-formatter/data-formatter-stl/libcxx/tuple/TestDataFormatterLibcxxTuple.py
lldb-api :: functionalities/data-formatter/data-formatter-stl/libcxx/unique_ptr/TestDataFormatterLibcxxUniquePtr.py
lldb-api :: functionalities/data-formatter/data-formatter-stl/libcxx/unordered/TestDataFormatterUnordered.py
lldb-api :: functionalities/data-formatter/data-formatter-stl/libcxx/variant/TestDataFormatterLibcxxVariant.py
lldb-api :: functionalities/data-formatter/data-formatter-stl/libcxx/vbool/TestDataFormatterLibcxxVBool.py
lldb-api :: functionalities/data-formatter/data-formatter-stl/libcxx/vector/TestDataFormatterLibcxxVector.py
lldb-api :: lang/cpp/std-function-step-into-callable/TestStdFunctionStepIntoCallable.py

25 tests on Windows:

Profile-x86_64 :: gcc-flag-compatibility.test
Profile-x86_64 :: instrprof-override-filename.c
cfi-standalone-lld-thinlto-x86_64 :: stats.cpp
cfi-standalone-lld-x86_64 :: stats.cpp
debuginfo-tests :: dexter/feature_tests/unittests/run.test
libomp :: affinity/format/affinity_values.c
libomp :: affinity/format/fields_values.c
libomp :: tasking/bug_nested_proxy_task.c
libomp :: tasking/bug_proxy_task_dep_waiting.c
lldb-shell :: SymbolFile/NativePDB/stack_unwinding01.cpp
lldb-shell :: SymbolFile/PDB/ast-restore.test
lldb-shell :: SymbolFile/PDB/class-layout.test
lldb-shell :: SymbolFile/PDB/enums-layout.test
lldb-shell :: SymbolFile/PDB/typedefs.test
lldb-shell :: SymbolFile/PDB/variables.test
lldb-unit :: Utility/./UtilityTests.exe/PassiveReplayTest.InstrumentedBar
lldb-unit :: Utility/./UtilityTests.exe/PassiveReplayTest.InstrumentedBarPtr
lldb-unit :: Utility/./UtilityTests.exe/PassiveReplayTest.InstrumentedBarRef
lldb-unit :: Utility/./UtilityTests.exe/PassiveReplayTest.InstrumentedFoo
lldb-unit :: Utility/./UtilityTests.exe/PassiveReplayTest.InstrumentedFooInvalid
lldb-unit :: Utility/./UtilityTests.exe/RecordReplayTest.InstrumentedBar
lldb-unit :: Utility/./UtilityTests.exe/RecordReplayTest.InstrumentedBarPtr
lldb-unit :: Utility/./UtilityTests.exe/RecordReplayTest.InstrumentedBarRef
lldb-unit :: Utility/./UtilityTests.exe/RecordReplayTest.InstrumentedFoo
lldb-unit :: Utility/./UtilityTests.exe/RecordReplayTest.InstrumentedFooSameThis

Thank you in advance!

I added some folks from LLDB that are more familiar with the depths of the expression parser and how the JIT are used. Jim, Fred and Jonas, do you guys have any ideas on wether this diff could affect the LLDB expression parser when using JIT?

I don't know how this JIT Profiling code is wired into the regular JIT mechanism. I assume it is opt-in, in which case it shouldn't have any effect on expression parsing. If it isn't opt in, we should have a way to opt out on the "don't do work whose outcome you don't care about" principle.

@jingham @clayborg Do either of you recognize the error that seems to be triggered? It looks like it's consistent across the tests, or at least the ones that I looked at:

FAIL: test_non_cpp_language_dwarf (TestImportStdModule.ImportStdModule)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/mnt/disks/ssd0/agent/llvm-project/lldb/packages/Python/lldbsuite/test/lldbtest.py", line 1825, in test_method
    return attrvalue(self)
  File "/mnt/disks/ssd0/agent/llvm-project/lldb/packages/Python/lldbsuite/test/decorators.py", line 134, in wrapper
    return func(*args, **kwargs)
  File "/mnt/disks/ssd0/agent/llvm-project/lldb/test/API/commands/expression/import-std-module/basic/TestImportStdModule.py", line 38, in test_non_cpp_language
    lldbutil.run_to_source_breakpoint(self,
  File "/mnt/disks/ssd0/agent/llvm-project/lldb/packages/Python/lldbsuite/test/lldbutil.py", line 953, in run_to_source_breakpoint
    return run_to_breakpoint_do_run(test, target, breakpoint, launch_info,
  File "/mnt/disks/ssd0/agent/llvm-project/lldb/packages/Python/lldbsuite/test/lldbutil.py", line 875, in run_to_breakpoint_do_run
    test.assertEqual(process.GetState(), lldb.eStateStopped)
AssertionError: 10 != 5

It's not clear to me that Elena's patch should affect this. I'm inclined to accept it and just keep an eye on the bots. If everything passes we can chalk it up to a misconfig on Harbormaster. If the bots fail we can revert, and maybe the logs will give us some more insight into what's going on.

@jingham @clayborg Do either of you recognize the error that seems to be triggered? It looks like it's consistent across the tests, or at least the ones that I looked at:

FAIL: test_non_cpp_language_dwarf (TestImportStdModule.ImportStdModule)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/mnt/disks/ssd0/agent/llvm-project/lldb/packages/Python/lldbsuite/test/lldbtest.py", line 1825, in test_method
    return attrvalue(self)
  File "/mnt/disks/ssd0/agent/llvm-project/lldb/packages/Python/lldbsuite/test/decorators.py", line 134, in wrapper
    return func(*args, **kwargs)
  File "/mnt/disks/ssd0/agent/llvm-project/lldb/test/API/commands/expression/import-std-module/basic/TestImportStdModule.py", line 38, in test_non_cpp_language
    lldbutil.run_to_source_breakpoint(self,
  File "/mnt/disks/ssd0/agent/llvm-project/lldb/packages/Python/lldbsuite/test/lldbutil.py", line 953, in run_to_source_breakpoint
    return run_to_breakpoint_do_run(test, target, breakpoint, launch_info,
  File "/mnt/disks/ssd0/agent/llvm-project/lldb/packages/Python/lldbsuite/test/lldbutil.py", line 875, in run_to_breakpoint_do_run
    test.assertEqual(process.GetState(), lldb.eStateStopped)
AssertionError: 10 != 5

That's the test failing to stop at the first breakpoint. 10 -> eStateExited, 5 -> eStateStopped. This sequence, build the binary then do:

lldbutil.run_to_source_breakpoint(self,
                                  "// Set break point at this line.",
                                  lldb.SBFileSpec("main.cpp"))

to set a breakpoint on that source pattern and run to it is how the vast majority of the Shell tests that actually run a target start up. Moreover, in this test we actually did manage to resolve the breakpoint, or the test would have failed earlier. So we must have had debug info we thought was okay.

IIRC the bots use the just-built clang as the builder for the test files, so it's possible that the presence of this patch in the clang we are using for building messes up the line table info so that the breakpoint ends up somewhere that doesn't correspond to code we actually run??? If you have a builder which still has the built products available, it would be interesting to grab one of the ones that is failing like this and try:

(lldb) break set -p "// Set break point at this line."
(lldb) run

and see if it hits the breakpoint.

It's not clear to me that Elena's patch should affect this. I'm inclined to accept it and just keep an eye on the bots. If everything passes we can chalk it up to a misconfig on Harbormaster. If the bots fail we can revert, and maybe the logs will give us some more insight into what's going on.

Hi Lang,

That's sounds good to me.

Btw, previously I mentioned, that could not reproduce the tests failures on my system with my patch applied(master from 9th September).
But, I tried to run tests on master revision 4ce176bed2c4f88804e7d4bb9671637d88206e78 without my patch and the same tests failures reproduced.
(this revision was chosen because it is near the time I created review on Phabricator)

I agree that I can't see how this would affect things. Fine with this going in and taking our chances.

I agree that I can't see how this would affect things. Fine with this going in and taking our chances.

Ok -- Sounds good to me. Elena -- You're welcome to land this, and thanks for being patient with the review.

Thanks a lot for your help!
Being very new here, I do not have commit access to land this, could you please land this?

Thanks a lot for your help!
Being very new here, I do not have commit access to land this, could you please land this?

Hi Elena,

I missed that this included a definition of a git submodule. This would be the first in the main LLVM tree and I think we'd need to ask the community about inclusion. Is it required for this patch, or could it be dropped and downloading of that module left to clients who want to enable Intel JIT events support?

Thanks a lot for your help!
Being very new here, I do not have commit access to land this, could you please land this?

Hi Elena,

I missed that this included a definition of a git submodule. This would be the first in the main LLVM tree and I think we'd need to ask the community about inclusion. Is it required for this patch, or could it be dropped and downloading of that module left to clients who want to enable Intel JIT events support?

Hi Lang!

At my opinion, to add submodule is a good solution for open source project to be up to date with ITT API source.
The alternative solution - just add necessary source files from ITT API to the llvm/lib/ExecutionEngine/IntelJITEvents/ directory and manually update them from time to time.
How do you think, what is better to implement in my case?

Thanks a lot for your help!
Being very new here, I do not have commit access to land this, could you please land this?

Hi Elena,

I missed that this included a definition of a git submodule. This would be the first in the main LLVM tree and I think we'd need to ask the community about inclusion. Is it required for this patch, or could it be dropped and downloading of that module left to clients who want to enable Intel JIT events support?

Hi Lang!

At my opinion, to add submodule is a good solution for open source project to be up to date with ITT API source.
The alternative solution - just add necessary source files from ITT API to the llvm/lib/ExecutionEngine/IntelJITEvents/ directory and manually update them from time to time.
How do you think, what is better to implement in my case?

I'm not opposed to submodules, but I think you would need to start a thread on LLVM dev about it: I'm not the right person to approve that change.

In the interim, if you would like to land the rest of this patch, do you think it would be reasonable to require clients using intel JIT events to manually create and checkout the submodule? I think that would only be a couple of extra lines when setting up an LLVM checkout, right?

ekovanov updated this revision to Diff 301729.Oct 29 2020, 1:35 PM

removed git submodule, added cmake action to clone ittapi repo

Thanks a lot for your help!
Being very new here, I do not have commit access to land this, could you please land this?

Hi Elena,

I missed that this included a definition of a git submodule. This would be the first in the main LLVM tree and I think we'd need to ask the community about inclusion. Is it required for this patch, or could it be dropped and downloading of that module left to clients who want to enable Intel JIT events support?

Hi Lang!

At my opinion, to add submodule is a good solution for open source project to be up to date with ITT API source.
The alternative solution - just add necessary source files from ITT API to the llvm/lib/ExecutionEngine/IntelJITEvents/ directory and manually update them from time to time.
How do you think, what is better to implement in my case?

I'm not opposed to submodules, but I think you would need to start a thread on LLVM dev about it: I'm not the right person to approve that change.

In the interim, if you would like to land the rest of this patch, do you think it would be reasonable to require clients using intel JIT events to manually create and checkout the submodule? I think that would only be a couple of extra lines when setting up an LLVM checkout, right?

Hi Lang,

I think I got your point and I fully agree with you - it does not necessary to create submodule.
An action to clone ITT API repo (CMakeLists.txt) is enough for this case.

Hi Lang,

Could you please take a look at the latest diff?
Is there something I should fix?

friendly reminder

lhames accepted this revision.Nov 15 2020, 6:49 PM

Hi Elena,

Sorry for the delayed reply -- I've been traveling and I'm still catching up. This looks good to me -- I'll land it now. :)

  • Lang.
This revision was landed with ongoing or failed builds.Nov 16 2020, 1:04 AM
This revision was automatically updated to reflect the committed changes.