[SYCL] Treat "ar" image format as PI_DEVICE_BINARY_TYPE_NATIVE #12587
Conversation
A future PR will add support for magic numbers other than four bytes. Refactor the code to make those future changes easier to review.
That's what AOT for Intel GPUs produces when targeting multiple devices at once.
aelovikov-intel
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
aelovikov-intel
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
sycl/source/detail/pi.cpp
Outdated
| if (ImgSize < sizeof(Number)) | ||
| return false; | ||
| return std::memcmp(ImgData, &Number, sizeof(Number)) == 0; |
There was a problem hiding this comment.
Nit, but I am not a huge fan of manual short-circuiting.
| if (ImgSize < sizeof(Number)) | |
| return false; | |
| return std::memcmp(ImgData, &Number, sizeof(Number)) == 0; | |
| return ImgSize >= sizeof(Number) && std::memcmp(ImgData, &Number, sizeof(Number)) == 0; |
sycl/source/detail/pi.cpp
Outdated
| // Check for ELF format, size requirements include data we'll read in case of | ||
| // succesful match. | ||
| if (ImgSize < 18 || !MatchMagicNumber(uint32_t{0x464c457F})) | ||
| return PI_DEVICE_BINARY_TYPE_NONE; |
There was a problem hiding this comment.
Nit: Though I am generally against nested nesting, a case like this I would rather we do the ELF header checks inside the if. It means when someone want to add more binary type detection, they don't have to realize that at this point we assume the remaining binary types must be ELF.
|
I just realized I reviewed the wrong PR. Alas, the changes can be applied to #12586. |
aelovikov-intel
had a problem deploying
to
WindowsCILock
GitHub Actions
Error
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
aelovikov-intel
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
aelovikov-intel
had a problem deploying
to
WindowsCILock
GitHub Actions
Failure
aelovikov-intel
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
aelovikov-intel
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
| // | ||
| // -Xsycl-target-backend=spir64_gen "-device acm-g10,acm-g11" | ||
| // | ||
| // option. |
There was a problem hiding this comment.
Small nit; I would prefer to have the comment outside the loop as having it inside the loop without brackets makes it hard to read... Unless you're used to Python code. 😉
There was a problem hiding this comment.
I'll keep as-is as it matches surrounding code better.
That's what AOT for Intel GPUs produces when targeting multiple devices
at once.
This PR is built on top of #12586.