Repository navigation
Conversation
…ds with 1-byte mem_base_addr_align
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
1 open issue
What changed in this PR
Updates the SYCL E2E buffer reinterpret tests to avoid creating zero-sized buffers on backends reporting very small mem_base_addr_align (e.g., Level Zero reporting 8 bits), enabling the test to run without XFAILs.
Changes:
- Removed XFAIL directives for Level Zero GPU and native CPU for this test.
- Updated sub-buffer offset/shape computations to use an alignment derived via
std::lcm(mem_base_align_in_bytes, sizeof(int)). - Fixed an incorrect assertion message for the 2D
char -> intcase.
| File | Description |
|---|---|
| sycl/test-e2e/Basic/buffer/reinterpret.cpp | Reworks alignment/offset math to prevent zero-sized buffers; removes XFAILs; corrects an assertion message. |
Review effort: Lite
| // RUN: %{build} -o %t.out | ||
| // RUN: %{run} %t.out | ||
| // | ||
| // XFAIL: level_zero&&gpu | ||
| // XFAIL-TRACKER: https://github.com/intel/llvm/issues/14430 | ||
| // XFAIL: target-native_cpu | ||
| // XFAIL-TRACKER: https://github.com/intel/llvm/issues/20142 | ||
|
|
There was a problem hiding this comment.
Yeah, the bug was also occuring on native cpu backend, but its one of the defects from that issue.
| const size_t sub_buf_align_in_bytes = | ||
| std::lcm(mem_base_align_in_bytes, sizeof(int)); | ||
| const size_t sub_buf_align_in_ints = sub_buf_align_in_bytes / sizeof(int); |
There was a problem hiding this comment.
The device alignment is reported in bytes, but this test uses it to calculate offsets and dimensions in int elements. With a 1-byte alignment (coming from e.g. Level Zero backend), dividing the raw value by sizeof(int) truncates to zero, which produces zero-sized dimensions. Taking the LCM gives a byte alignment that is both a valid multiple of the device requirement and divisible by sizeof(int), so those calculations remain nonzero and correctly aligned.
Fixes issue: #14430, and one of the defects from #20142
Description:
The test assumed that
info::device::mem_base_addr_alignis at least sizeof(int) and divisible by it. This is true on OpenCL, CUDA and HIP backends, but not on Level Zero, which currently report an alignment of 8 bits.The old computation performed in the reintepret.cpp tests could produce zero-sized buffers and invalid char <-> int reinterpretation ranges when the reported alignment was smaller than sizeof(int).