-
Notifications
You must be signed in to change notification settings - Fork 796
[SYCL][E2E] Update is_compatible tests #17606
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
Conversation
Due to incorrect REQUIRES these tests are never launched.
| Compatible &= Dev.is_gpu(); | ||
| Called = true; | ||
| } | ||
| if (sycl::is_compatible<KernelACC>(Dev)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing this since there is no chance now to launch this on fpga.
|
@KseniyaTikhomirova take a look please |
KseniyaTikhomirova
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
| // There are no ROCm libs on win machines, so the compilation fails. | ||
|
|
||
| // RUN: %clangxx -fsycl -Xsycl-target-backend=amdgcn-amd-amdhsa --offload-arch=gfx906 -fsycl-targets=amdgcn-amd-amdhsa %S/Inputs/is_compatible_with_env.cpp -o %t.out | ||
| // RUN: %clangxx -fsycl -fsycl-targets=amdgcn-amd-amdhsa -Xsycl-target-backend=amdgcn-amd-amdhsa --offload-arch=gfx1030 %S/Inputs/is_compatible_with_env.cpp -o %t.out |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a necessity to specify --offload-arch? it is absent for cuda, maybe can be skipped here either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree it may lead to a failure in the future if we change the hardware, but It looks like currently we don't specify the offload-arch automatically, need to extend the testing script. @intel/dpcpp-devops-reviewers am I missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps you could use %{hip_arch_opts}
see this bit,
Lines 239 to 246 in 0f42c75
| if "target-amd" in build_targets: | |
| hip_arch_opts = ( | |
| " -Xsycl-target-backend=amdgcn-amd-amdhsa --offload-arch={}".format( | |
| test.config.amd_arch | |
| ) | |
| ) | |
| sycl_target_opts += hip_arch_opts | |
| substitutions.append(("%{hip_arch_opts}", hip_arch_opts)) |
Perhaps this could also be guarded by REQUIRES: any-target-is-amd instead of doing UNSUPPORTED: windows?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps you could use %{hip_arch_opts}
As I understand hip_arch_opts works only with target-amd, so it won't be substituted otherwise.
this could also be guarded by REQUIRES: any-target-is-amd
the intention of this test is to be launched on other than amd platform to check if the is_compatible<kernel>(dev) call returns false if the kernel was built for another device.
|
@intel/llvm-gatekeepers could you please merge? |
updates UNSUPPORTED added in intel#17606
Due to incorrect REQUIRES these tests are never launched.