Page MenuHomePhabricator

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

Authored by ekovanov on Sun, Nov 22, 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.Sun, Nov 22, 10:17 AM
Herald added a project: Restricted Project. · View Herald TranscriptSun, Nov 22, 10:17 AM
ekovanov requested review of this revision.Sun, Nov 22, 10:17 AM
etyurin accepted this revision.Mon, Nov 23, 12:32 AM

LGTM.

This revision is now accepted and ready to land.Mon, Nov 23, 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.Tue, Nov 24, 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.Tue, Nov 24, 3:56 AM
bader added a comment.Tue, Nov 24, 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.Tue, Nov 24, 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.Tue, Nov 24, 10:40 AM

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

This revision is now accepted and ready to land.Tue, Nov 24, 10:40 AM
tstellar requested changes to this revision.Tue, Nov 24, 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.Tue, Nov 24, 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..Tue, Nov 24, 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.EditedTue, Nov 24, 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.Wed, Nov 25, 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.