Skip to content
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

[CI] Enable FPGA on postcommit for Linux #12673

Merged
merged 14 commits into from
Feb 21, 2024

Conversation

uditagarwal97
Copy link
Contributor

@uditagarwal97 uditagarwal97 commented Feb 8, 2024

This PR enables testing on opencl::fpga for Linux in postcommit.
I have currently disabled the tests that were failing on the fpga and created another Github issue for the same: #12683. I am not very sure if the failures are because of some implementation bug or if that feature is not supported on fpga.

@uditagarwal97 uditagarwal97 self-assigned this Feb 9, 2024
@uditagarwal97 uditagarwal97 added the github_actions Pull requests that update GitHub Actions code label Feb 9, 2024
@uditagarwal97 uditagarwal97 changed the title [CI] [DRAFT] Enable FPGA on precommit for Linux [CI] Enable FPGA on precommit for Linux Feb 9, 2024
Copy link
Contributor

@Alcpz Alcpz 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 I found out the issue. We instantiate kernels with both 32bit and 64 bit datatypes. When the JIT compilation gets called the first time, all kernels get incorporated to the same module for compilation, triggering the issue.

-fsycl-device-code-split=per_kernel splits each kernel into independent modules, which should fix the issue.

Edit: @aelovikov-intel the query is working as intended.

sycl/test-e2e/syclcompat/atomic/atomic_arith.cpp Outdated Show resolved Hide resolved
sycl/test-e2e/syclcompat/atomic/atomic_comp_exchange.cpp Outdated Show resolved Hide resolved
@aelovikov-intel
Copy link
Contributor

I think I found out the issue. We instantiate kernels with both 32bit and 64 bit datatypes. When the JIT compilation gets called the first time, all kernels get incorporated to the same module for compilation, triggering the issue.

-fsycl-device-code-split=per_kernel splits each kernel into independent modules, which should fix the issue.

Edit: @aelovikov-intel the query is working as intended.

That should be handled by Optional Kernel Features (https://github.com/intel/llvm/blob/sycl/sycl/doc/design/OptionalDeviceFeatures.md), shouldn't it? In particular, I'd expect two device images created even without that option. I admit that I haven't worked closely with that feature, so maybe I'm missing something.

+ @AlexeySachkov @steffenlarsen

@steffenlarsen
Copy link
Contributor

steffenlarsen commented Feb 15, 2024

@aelovikov-intel is right, the module-splitting done by sycl-post-commit should split kernels using atomic64 into a separate module. Since the failure seems to be coming from the online compiler and not from our runtime, which it should, I suspect we have a case of atomics on 64-bit values that aren't appropriately marked as [[__sycl_detail__::__uses_aspects__(aspect::atomic64)]].

I suggest you open an issue here on Github with it, add per_kernel splitting to the test, and add a comment that it's a bug due to the newly opened issue.

@uditagarwal97 uditagarwal97 changed the title [CI] Enable FPGA on precommit for Linux [CI] Enable FPGA on postcommit for Linux Feb 20, 2024

// TODO: Remove unsupported after fixing
// https://github.com/intel/llvm/issues/12683
// UNSUPPORTED: accelerator
Copy link
Contributor

Choose a reason for hiding this comment

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

accelerator or fpga?

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 think it's accelerator only because that's what the name of the feature is (https://github.com/intel/llvm/blob/sycl/sycl/test-e2e/lit.cfg.py#L678) when fpga device is reported by sycl-ls.

@uditagarwal97
Copy link
Contributor Author

The precommit failure in ESIMD/unified_memory_api/scatter_lacc_dg2_pvc.cpp seems unrealted: #12769

@aelovikov-intel aelovikov-intel merged commit 69d233b into intel:sycl Feb 21, 2024
18 of 19 checks passed
@uditagarwal97 uditagarwal97 deleted the enable_fpga branch March 21, 2024 19:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
github_actions Pull requests that update GitHub Actions code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants