Skip to content

[SYCL][ESIMD][EMU] pi_esimd_cpu bringing up with CM library #4011

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 11 commits into from
Aug 27, 2021

Conversation

dongkyunahn-intel
Copy link
Contributor

No description provided.

@dongkyunahn-intel
Copy link
Contributor Author

This PR is for a change set that invokes CM functionalities for ESIMD_CPU - initializing CM_EMU module, launching kernel, and accessing CM-managed resources like buffer, etc. CM_EMU library is built online downloading opensource CM_EMU hosted in github.

dongkyunahn-intel added a commit to dongkyunahn-intel/llvm that referenced this pull request Jun 29, 2021
* This PR is for enabling kernel launching for ESIMD_CPU

* Also contains emulated intrinsics for memory operations

* esimd_cpu backend is loaded in SYCL runtime

* Base PR : intel#4011
dongkyunahn-intel added a commit to dongkyunahn-intel/llvm that referenced this pull request Jun 29, 2021
* This PR is for enabling kernel launching for ESIMD_CPU

* Also contains emulated intrinsics for memory operations

* esimd_cpu backend is loaded in SYCL runtime

* Base PR : intel#4011
@kbobrovs kbobrovs requested a review from romanovvlad June 29, 2021 15:14
Copy link
Contributor

@kbobrovs kbobrovs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First few comments, continuing...

Copy link
Contributor

@kbobrovs kbobrovs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

continuing...

Copy link
Contributor

@kbobrovs kbobrovs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

continuing...

Copy link
Contributor

@kbobrovs kbobrovs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

may have few more comments after these ones are addressed

smaslov-intel
smaslov-intel previously approved these changes Jul 12, 2021
Copy link
Contributor

@smaslov-intel smaslov-intel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PI related changes LGTM

@dongkyunahn-intel
Copy link
Contributor Author

This PR cannot be merged automatically due to conflict from #4014 (File relocation + revision). I'll make a single commit using these change sets and fix conflict on top of it.

Copy link
Contributor

@kbobrovs kbobrovs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please address FIXME in piMemBufferCreate in this PR, and my 8 other comments could be addresses in a separate PR

Copy link
Contributor

@romanovvlad romanovvlad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is the final set of comments.

romanovvlad
romanovvlad previously approved these changes Jul 28, 2021
@dongkyunahn-intel
Copy link
Contributor Author

As two reviewers have approved this PR, I'll resolve conflict in order to merge to SYCL branch. Some changes could be reverted and brought back with revision aligned with up to date SYCL branch.

- This PR is for a change set that invokes CM functionalities for
ESIMD_CPU - initializing CM_EMU module, launching kernel, and
accessing CM-managed resources like buffer, etc. CM_EMU library is
built online downloading opensource CM_EMU hosted in
github (https://github.com/intel/cm-cpu-emulation.git) under Linux
environment

Co-authored-by: Romanov Vlad <[email protected]>
Co-authored-by: kbobrovs <[email protected]>
@DenisBakhvalov DenisBakhvalov self-requested a review August 17, 2021 21:46
DenisBakhvalov
DenisBakhvalov previously approved these changes Aug 17, 2021
@kbobrovs
Copy link
Contributor

Per discussion with @dongkyunahn-intel, we should not build any dependencies like libCM or moreover libva/libdrm. ESIMD EMU cmake files should only depend on pre-built libCM.

We should also understand what are runtime dependencies for the libCM version we build the plugin with, and have those (libva, libdrm,...) installed on the buildbot machines which run ESIMD CPU emulation tests.

- Instead of building online
- Library package is downloaded from
https://github.com/intel/cm-cpu-emulation
- Windows is not supported yet
@pvchupin
Copy link
Contributor

Per discussion with @dongkyunahn-intel, we should not build any dependencies like libCM or moreover libva/libdrm. ESIMD EMU cmake files should only depend on pre-built libCM.

We should also understand what are runtime dependencies for the libCM version we build the plugin with, and have those (libva, libdrm,...) installed on the buildbot machines which run ESIMD CPU emulation tests.

I think we need a full list of new deps like you said, also we need to see if these are available from the standard OS distro or should be taken from elsewhere? What's the plan for Windows?

@dongkyunahn-intel
Copy link
Contributor Author

I think we need a full list of new deps like you said, also we need to see if these are available from the standard OS distro or should be taken from elsewhere? What's the plan for Windows?

As far as I know, open-source CM has dependency on libva and libffi. Libva has dependency on libdrm and some utilities like autoconf. Some packages and utilities are not installed by default under ubuntu - they have to be installed manually.

For Windows, CM has dependency on libffi. As far as I know, there is no pre-built library for libffi.dll/lib we can download from internet. I was considering pre-built package for Windows generated in-house as building libffi during CM building with MSVC was tricky. There could be more dependency as Windows opensource work is not started yet.

@dongkyunahn-intel dongkyunahn-intel dismissed kbobrovs’s stale review August 26, 2021 14:58

Github cannot find requested change. Probably removed during forced-push.

Copy link
Contributor

@kbobrovs kbobrovs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to go to me, towards initial bring-up of the feature.

@dongkyunahn-intel
Copy link
Contributor Author

@kbobrovs,

Github still says 'Merging is blocked' / 'The base branch restricts merging to authorized users'. I got approvals from two code owners - Kai Yu and Sergey M, and all checks have passed. Can you help me merge this PR?

@bader bader requested a review from romanovvlad August 27, 2021 06:03
@bader
Copy link
Contributor

bader commented Aug 30, 2021

@dongkyunahn-intel, I reverted this change because it failed build in post-commit testing: https://github.com/intel/llvm/runs/3442475454.
Please, fix this problem and open new PR.

@dongkyunahn-intel
Copy link
Contributor Author

@dongkyunahn-intel, I reverted this change because it failed build in post-commit testing: https://github.com/intel/llvm/runs/3442475454.
Please, fix this problem and open new PR.

@bader, this problem has to be resolved in infra side. The failure is because of missing package - libva-dev(ubuntu) / libva-devel(RHEL). I had same problem with RHEL build from Jenkins/Precommit test, and it was resolved with libva-devel package installed. CM_EMU has dependency on the libva-dev library. I'll talk to Konst and infra folks.

@bader
Copy link
Contributor

bader commented Aug 30, 2021

@dongkyunahn-intel, this is a new requirement for development system, which impacts all developers.
Is this new library linked dynamically, so our users will have to install it as well?

@dongkyunahn-intel
Copy link
Contributor Author

@bader,

Yes. CM requires libva library installed - https://github.com/intel/cm-cpu-emulation#linux. (Although there is other dependencies in the documentation, they are not required for ESIMD_CPU).

@bader
Copy link
Contributor

bader commented Aug 30, 2021

Can we follow CUDA/HIP plug-in approach and enable ESIMD_CPU only if its build is explicitly requested via ./configure.py script flag?

@pvchupin, FYI.

@dongkyunahn-intel
Copy link
Contributor Author

@bader,

Yes. I can disable ESIMD_CPU by default in configure.py script and make build procedure build it only requested with a command line option like --enable-esimd-cpu.

@dongkyunahn-intel
Copy link
Contributor Author

New PR posted : #4430

vmaksimo pushed a commit to vmaksimo/llvm that referenced this pull request Sep 1, 2021
Reverted intel#4011 as part of the resolution (which would be reverted
otherwise in 2 commits) to fix the build. Essentially a cherry-pick of:

9189ef6 Mon Aug 30 01:59:34 2021  Alexey Bader:  Revert
"[SYCL][ESIMD][EMU] pi_esimd_cpu bringing up with CM library (intel#4011)"
(intel#4421)
kbobrovs pushed a commit that referenced this pull request Nov 30, 2021
…ing (#4020)

* [SYCL][ESIMD][EMU] ESIMD_CPU Kernel launch and Emulated Intrinsics

* This PR is for enabling kernel launching for ESIMD_CPU
* esimd_cpu backend is loaded in SYCL runtime
* Base PR : #4011

Author: dongkyunahn-intel <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants