Skip to content

[OpenCL] Add OpenCL CPU RT code #8216

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

Closed
wants to merge 0 commits into from
Closed

Conversation

cdai2
Copy link
Contributor

@cdai2 cdai2 commented Feb 6, 2023

It includes the OpenCL CPU RT, Transform passes to support SYCL and OCL and LIT test cases for them.
.

@cdai2 cdai2 requested review from a team and bader as code owners February 6, 2023 17:55
@cdai2 cdai2 requested a review from steffenlarsen February 6, 2023 17:55
@cdai2
Copy link
Contributor Author

cdai2 commented Feb 6, 2023

The compilation support for this code is not included yet. We will submit it in another patch. It only contains the code we want to land.

@cdai2 cdai2 requested a review from romanovvlad February 6, 2023 18:04
@cdai2 cdai2 temporarily deployed to aws February 6, 2023 18:24 — with GitHub Actions Inactive
@cdai2 cdai2 requested a review from a team as a code owner February 7, 2023 06:10
@cdai2 cdai2 temporarily deployed to aws February 7, 2023 06:28 — with GitHub Actions Inactive
@cdai2 cdai2 requested a review from a team as a code owner February 7, 2023 07:46
@cdai2 cdai2 temporarily deployed to aws February 7, 2023 08:03 — with GitHub Actions Inactive
@cdai2 cdai2 temporarily deployed to aws February 7, 2023 18:16 — with GitHub Actions Inactive
@bader bader linked an issue Feb 7, 2023 that may be closed by this pull request

if args.bldocl or args.bldfpgaemu:
llvm_external_projects += ';opencl-cpu'

Copy link
Contributor

Choose a reason for hiding this comment

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

These components should be enabled when args.ci_defaults is true to build them in CI.

Copy link
Contributor

Choose a reason for hiding this comment

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

@cdai2, please, address this comment.

I also recommend moving "Transform passes to support SYCL and OCL and LIT test cases for them" to a separate patch. It doesn't require changes in build scripts except CMake changes and will make it easier to review.

Please, provide instructions for reviewers how to review changes. GitHub is not able to display the full diff.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to enable opencl-cpu build under args.ci_defaults arguments according to my understanding. Please correct me if it's not true.

Copy link
Contributor Author

@cdai2 cdai2 Feb 8, 2023

Choose a reason for hiding this comment

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

#8250 is created to only upload SYCLTransform passes and LIT test. Please review. I will remove this part from this #8216 PR once you approve #8250.

As this PR is to upload these files for the first time. I have no idea to display the diff for it.

Copy link
Contributor

Choose a reason for hiding this comment

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

As this PR is to upload these files for the first time. I have no idea to display the diff for it.

I think a major part of the diff is the content of opencl/ directory. I suggest we keep changes in files outside of this directory in separate commits and you can ask code owners to review specific commits only and ignore the commits with changes in opencl/ directory. I assume that @intel/ocl-cpu-rt-write team is already reviewed the content of the opencl/ directory and can approve it. If not, the only idea I have is to pull the source code locally and review it locally.

@cdai2 cdai2 force-pushed the upload_opencl_cpu_code branch from 9b0191f to 7de27eb Compare February 8, 2023 00:46
@cdai2 cdai2 temporarily deployed to aws February 8, 2023 01:11 — with GitHub Actions Inactive
@cdai2 cdai2 force-pushed the upload_opencl_cpu_code branch from 7de27eb to 107358d Compare February 8, 2023 03:27
@cdai2 cdai2 requested a review from a team as a code owner February 8, 2023 03:27
@wenju-he
Copy link
Contributor

wenju-he commented Feb 8, 2023

@cdai2 pass registration code isn't uploaded yet?

@cdai2 cdai2 temporarily deployed to aws February 8, 2023 03:45 — with GitHub Actions Inactive
@cdai2
Copy link
Contributor Author

cdai2 commented Feb 8, 2023

@cdai2 pass registration code isn't uploaded yet?

landed again.

@cdai2 cdai2 temporarily deployed to aws February 8, 2023 04:19 — with GitHub Actions Inactive
@cdai2 cdai2 temporarily deployed to aws February 8, 2023 04:44 — with GitHub Actions Inactive
@Nuullll Nuullll temporarily deployed to aws February 8, 2023 05:25 — with GitHub Actions Inactive
@cdai2 cdai2 temporarily deployed to aws February 8, 2023 14:31 — with GitHub Actions Inactive
@bader
Copy link
Contributor

bader commented Feb 10, 2023

@bader Could you please help to remove unnecessary reviewers? I did a rebase and squashed my two commits. Then a lot of commits are contained and introduced these reviewers.

Do you mean you can't remove them?

@cdai2 cdai2 temporarily deployed to aws February 10, 2023 19:27 — with GitHub Actions Inactive
@cdai2 cdai2 requested review from bader and a team and removed request for a team, tfzhu, victor-eds, steffenlarsen, Naghasan and sommerlukas February 11, 2023 02:08
@cdai2 cdai2 temporarily deployed to aws February 15, 2023 11:21 — with GitHub Actions Inactive
@jbrodman
Copy link
Contributor

Why are there binary .dlls included in this PR?

@jbrodman
Copy link
Contributor

There are also several external dependencies that have been copied into this source tree. Shouldn't these be dynamically pulled in by the configuration step?

@cdai2 cdai2 closed this Feb 18, 2023
@cdai2 cdai2 deleted the upload_opencl_cpu_code branch February 18, 2023 02:13
@cdai2 cdai2 restored the upload_opencl_cpu_code branch February 18, 2023 02:28
@cdai2 cdai2 reopened this Feb 18, 2023
@cdai2 cdai2 closed this Feb 18, 2023
@cdai2 cdai2 force-pushed the upload_opencl_cpu_code branch from fe9a8fe to 3e5820a Compare February 18, 2023 02:50
@cdai2
Copy link
Contributor Author

cdai2 commented Feb 20, 2023

Why are there binary .dlls included in this PR?

They are generated during compilation for test cases. We will remove. thanks

@cdai2
Copy link
Contributor Author

cdai2 commented Feb 20, 2023

There are also several external dependencies that have been copied into this source tree. Shouldn't these be dynamically pulled in by the configuration step?

Yes, We have plan to pull in them during configuration step. we will update it. thanks

@intel intel deleted a comment from ValeZAA Jan 6, 2025
@intel intel locked and limited conversation to collaborators Jan 6, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Experimental CPU Runtime] Request for open sourcing the CPU RT
5 participants