8000 [SYCL] Fix amdgpu openmp test by npmiller · Pull Request #5103 · intel/llvm · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

[SYCL] Fix amdgpu openmp test #5103

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Dec 8, 2021
Merged

[SYCL] Fix amdgpu openmp test #5103

merged 1 commit into from
Dec 8, 2021

Conversation

npmiller
Copy link
Contributor
@npmiller npmiller commented Dec 8, 2021

The -target parameter of the offload wrapper was renamed to -host a
while back on the SYCL branch.

This fixes one of the issues seen in #5087

@npmiller
[SYCL] Fix amdgpu openmp test
a2507d2 
The `-target` parameter of the offload wrapper was renamed to `-host` a
while back on the SYCL branch.

This fixes one of the issues seen in intel#5087
@bader bader requested a review from sndmitriev December 8, 2021 10:51
AGindinson
AGindinson approved these changes Dec 8, 2021
View reviewed changes
clang/test/Driver/amdgpu-openmp-toolchain.c
@@ -10,7 +10,7 @@
// CHECK: llvm-link{{.*}}"-o" "{{.*}}amdgpu-openmp-toolchain-{{.*}}-gfx906-linked-{{.*}}.bc"
// CHECK: llc{{.*}}amdgpu-openmp-toolchain-{{.*}}-gfx906-linked-{{.*}}.bc" "-mtriple=amdgcn-amd-amdhsa" "-mcpu=gfx906" "-filetype=obj" "-o"{{.*}}amdgpu-openmp-toolchain-{{.*}}-gfx906-{{.*}}.o"
// CHECK: lld{{.*}}"-flavor" "gnu" "--no-undefined" "-shared" "-o"{{.*}}amdgpu-openmp-toolchain-{{.*}}.out" "{{.*}}amdgpu-openmp-toolchain-{{.*}}-gfx906-{{.*}}.o"
// CHECK: clang-offload-wrapper{{.*}}"-target" "x86_64-unknown-linux-gnu" "-o" "{{.*}}a-{{.*}}.bc" {{.*}}amdgpu-openmp-toolchain-{{.*}}.out"
// CHECK: clang-offload-wrapper{{.*}}"-host" "x86_64-unknown-linux-gnu" "-o" "{{.*}}a-{{.*}}.bc" {{.*}}amdgpu-openmp-toolchain-{{.*}}.out"
Copy link
Contributor

Choose a reason for hiding this comment

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

Would we want to double-check the device target here? (a non-insisting suggestion)

Suggested change
// CHECK: clang-offload-wrapper{{.*}}"-host" "x86_64-unknown-linux-gnu" "-o" "{{.*}}a-{{.*}}.bc" {{.*}}amdgpu-openmp-toolchain-{{.*}}.out"
// CHECK: clang-offload-wrapper{{.*}}"-host" "x86_64-unknown-linux-gnu" "-o" "{{.*}}a-{{.*}}.bc" {{.*}}"-target=amdgcn-amd-amdhsa" {{.*}}amdgpu-openmp-toolchain-{{.*}}.out"

Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest we don't make this change in sycl branch to minimize the divergence with the main branch.
We can consider improving the test in llorg instead.

@bader
Copy link
Contributor
bader commented Dec 8, 2021

The -target parameter of the offload wrapper was renamed to -host a
while back on the SYCL branch.

@sndmitriev, do you plan to upstream this change to llorg?

@sndmitriev
Copy link
Contributor

The -target parameter of the offload wrapper was renamed to -host a
while back on the SYCL branch.

@sndmitriev, do you plan to upstream this change to llorg?

Shouldn't it be done by the author of that change?

@bader
Copy link
Contributor
bader commented Dec 8, 2021

Shouldn't it be done by the author of that change?

I thought that you as code owner should know about that. Do know know who made this change in the sycl branch?

@sndmitriev
Copy link
Contributor

Shouldn't it be done by the author of that change?

I thought that you as code owner should know about that. Do know know who made this change in the sycl branch?

I think this option was renamed in the following commit 0738099

@bader
Copy link
Contributor
bader commented Dec 8, 2021

Shouldn't it be done by the author of that change?

I thought that you as code owner should know about that. Do know know who made this change in the sycl branch?

I think this option was renamed in the following commit 0738099

@kbobrovs, when do you plan to commit this patch to llorg? We need this to reduce the maintenance cost for the sycl branch.

@bader bader merged commit 0070e9e into intel:sycl Dec 8, 2021
@bader
Copy link
Contributor
bader commented Dec 8, 2021

@npmiller, thanks a lot for fixing this issue!

@bader bader mentioned this pull request Dec 8, 2021
Merged
@kbobrovs
Copy link
Contributor
kbobrovs commented Dec 9, 2021

@kbobrovs, when do you plan to commit this patch to llorg? We need this to reduce the maintenance cost for the sycl branch.

I did not have such plans until now. Will see if I can do this sometime in January.

alexbatashev added a commit to alexbatashev/llvm that referenced this pull request Dec 11, 2021
@alexbatashev
Merge remote-tracking branch 'upstream/sycl' into hip_cuda_builds
f7efebe 
* upstream/sycl: (725 commits)
  [SYCL] Translate ZE_RESULT_ERROR_INVALID_ARGUMENT error code from L0 RT (intel#5122)
  [SYCL][L0][Plugin] Call ZeCommandQueueCreate on demand 
8000
(intel#5109)
  [SYCL] Switch to using blocking USM free for OpenCL GPU (intel#4928)
  [CI] Disable pack and upload steps (intel#5119)
  [SYCL] Disable submission of AssertInfoCopier for FPGA (intel#4780)
  [SYCL][SPIRV] Implement islessgreater with FOrdNotEqual instead (intel#5076)
  [SYCL] Fix typo in the name of the host-visible pool (intel#5073)
  [SYCL] Only call shutdown when DLL is being unloaded, not when process is terminating (intel#4983)
  [SYCL][CUDA][PI] Fix infinite loop when parallel_for range exceeds INT_MAX (intel#5095)
  [SYCL] Translate out-of-memory error codes from L0 RT (intel#5107)
  [SYCL] Fix a few warnings during build scripts configuration (intel#5082)
  [SYCL] Fix amdgpu openmp test (intel#5103)
  [SYCL] [FPGA] Create experimental headers for FPGA latency control (intel#5066)
  [SYCL][CUDA] Don't enqueue an event wait on same CUDA stream (intel#5099)
  Remove PR disable template (intel#5102)
  [BuildBot]Uplift CPU/FPGAEMU RT version (intel#5078)
  [SYCL] Fix the test to not depend on a specific line. (intel#5092)
  [CI] Provide libclc targets to build and test (intel#5091)
  Fix build of `check-llvm-spirv` target after 8f8001a
  Force opt to use new pass manager in pr52289 test after c34d157
  ...
alexbatashev added a commit to alexbatashev/llvm that referenced this pull request Dec 12, 2021
@alexbatashev
Merge remote-tracking branch 'upstream/sycl' into llvm_test_suite
ec072b1 
* upstream/sycl:
  [CI] Add container users to video group (intel#5101)
  [CI] More typo fixes in Nightly build (intel#5088)
  Revert "[CI] Disable pack and upload steps (intel#5119)" (intel#5125)
  [SYCL] Translate ZE_RESULT_ERROR_INVALID_ARGUMENT error code from L0 RT (intel#5122)
  [SYCL][L0][Plugin] Call ZeCommandQueueCreate on demand (intel#5109)
  [SYCL] Switch to using blocking USM free for OpenCL GPU (intel#4928)
  [CI] Disable pack and upload steps (intel#5119)
  [SYCL] Disable submission of AssertInfoCopier for FPGA (intel#4780)
  [SYCL][SPIRV] Implement islessgreater with FOrdNotEqual instead (intel#5076)
  [SYCL] Fix typo in the name of the host-visible pool (intel#5073)
  [SYCL] Only call shutdown when DLL is being unloaded, not when process is terminating (intel#4983)
  [SYCL][CUDA][PI] Fix infinite loop when parallel_for range exceeds INT_MAX (intel#5095)
  [SYCL] Translate out-of-memory error codes from L0 RT (intel#5107)
  [SYCL] Fix a few warnings during build scripts configuration (intel#5082)
  [SYCL] Fix amdgpu openmp test (intel#5103)
  [SYCL] [FPGA] Create experimental headers for FPGA latency control (intel#5066)
  [SYCL][CUDA] Don't enqueue an event wait on same CUDA stream (intel#5099)
  Remove PR disable template (intel#5102)
  [BuildBot]Uplift CPU/FPGAEMU RT version (intel#5078)
@bader
Copy link
Contributor
bader commented Feb 1, 2022

@kbobrovs, when do you plan to commit this patch to llorg? We need this to reduce the maintenance cost for the sycl branch.

I did not have such plans until now. Will see if I can do this sometime in January.

@kbobrovs, do you have any updates regarding upstreaming 0738099?

@kbobrovs
Copy link
Contributor
kbobrovs commented Feb 1, 2022
< 8F89 tbody class="d-block">

@bader, no, not yet unfortunately. It is quite a bit of work, no resources for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bader bader bader left review comments

@AGindinson AGindinson AGindinson approved these changes

@hchilama hchilama Awaiting requested review from hchilama

@mdtoguchi mdtoguchi Awaiting requested review from mdtoguchi

@sndmitriev sndmitriev Awaiting requested review from sndmitriev

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@npmiller @bader @sndmitriev @kbobrovs @AGindinson
0