This is an archive of the discontinued LLVM Phabricator instance.

[MCJIT] Add cmake variables to customize ittapi git location and revision.
ClosedPublic

Authored by ekovanov on Nov 22 2020, 10:17 AM.

Details

Summary

To support llorg builds this patch provides the following changes:

  1. Added cmake variable ITTAPI_GIT_REPOSITORY to control the location of ITTAPI repository. Default value of ITTAPI_GIT_REPOSITORY is github location: https://github.com/intel/ittapi.git Also, the separate cmake variable ITTAPI_GIT_TAG was added for repo tag.
  2. Added cmake variable ITTAPI_SOURCE_DIR to control the place where the repo will be cloned. Default value of ITTAPI_SOURCE_DIR is build area: PROJECT_BINARY_DIR

Diff Detail

Event Timeline

ekovanov created this revision.Nov 22 2020, 10:17 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 22 2020, 10:17 AM
ekovanov requested review of this revision.Nov 22 2020, 10:17 AM
etyurin accepted this revision.Nov 23 2020, 12:32 AM

LGTM.

This revision is now accepted and ready to land.Nov 23 2020, 12:32 AM

Hi @etyurin ,
I do not have commit access - could you please land it?

Hi @etyurin ,
I do not have commit access - could you please land it?

I don't have it as well:)

bader requested changes to this revision.Nov 24 2020, 3:56 AM
bader added a subscriber: bader.

I also suggest using CMake's ExternalProject_Add command to checkout sources instead of running a set of execute_process commands.

llvm/lib/ExecutionEngine/IntelJITEvents/CMakeLists.txt
14–15

CMAKE_CURRENT_SOURCE_DIR -> ITTAPI_SOURCE_DIR

This revision now requires changes to proceed.Nov 24 2020, 3:56 AM
bader added a comment.Nov 24 2020, 3:57 AM

Hi @etyurin ,
I do not have commit access - could you please land it?

I can help with that, but current version of the patch has a bug, which should be addressed before commit.

ekovanov updated this revision to Diff 307409.Nov 24 2020, 10:37 AM

fixed llvm/lib/ExecutionEngine/IntelJITEvents/CMakeLists.txt:15
CMAKE_CURRENT_SOURCE_DIR -> ITTAPI_SOURCE_DIR

thank you Alexey!

bader accepted this revision.Nov 24 2020, 10:40 AM

@etyurin, let me know when it's ready for commit.

This revision is now accepted and ready to land.Nov 24 2020, 10:40 AM
tstellar requested changes to this revision.Nov 24 2020, 10:42 AM
tstellar added a subscriber: tstellar.

Can you explain more about what llorg is and why this change is needed?

This revision now requires changes to proceed.Nov 24 2020, 10:42 AM

Can you explain more about what llorg is and why this change is needed?

llorg is the term used by Intel compiler team to differentiate vanilla llvm source repository (or compiler built from these sources) from Intel's customized version. It shouldn't have been used w/o providing the context.

This change allows developers to customize ittapi git repository location and source revision. It's required for enabling use of internal mirrors.

I suggest changing the log message title to:

  • [MCJIT] Add cmake variables to customize ittapi git location and revision.
ekovanov retitled this revision from [MCJIT]: Add cmake variables to support llorg builds to [MCJIT] Add cmake variables to customize ittapi git location and revision..Nov 24 2020, 8:10 PM

Thanks for the explanation. Doing a git clone as part of the build is usually discouraged, though I see Lang approved the original patch. Is this code disabled by default?

ekovanov marked an inline comment as done.EditedNov 24 2020, 8:36 PM

Hi Tom,
Yes, it's disabled by default. To enable this code the option -DLLVM_USE_INTEL_JITEVENTS=ON should be used.

etyurin accepted this revision.Nov 25 2020, 1:24 AM

Thanks for the explanation. Doing a git clone as part of the build is usually discouraged, though I see Lang approved the original patch. Is this code disabled by default?

I approved under the understanding that this was off by default, but I'm definitely not a build system expert -- If you feel this should be done differently then that seems worth discussing.

bader added a comment.Dec 9 2020, 3:08 AM

@tstellar, are you okay with the change?
Please, let us know if you have any additional questions.

In my opinion , it would be better to to build against an installed version of the library, rather than fetching the git source. But I don't have any objections to this specific patch, because it is updating something that is already in tree.

bader added a comment.Dec 9 2020, 9:00 AM

In my opinion , it would be better to to build against an installed version of the library, rather than fetching the git source. But I don't have any objections to this specific patch, because it is updating something that is already in tree.

Thanks! I'll submit this patch.
@ekovanov, please, consider building against installed version of the library.

This revision was not accepted when it landed; it landed in state Needs Revision.Dec 9 2020, 10:24 AM
This revision was automatically updated to reflect the committed changes.