-
Notifications
You must be signed in to change notification settings - Fork 736
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
[SYCL] Select device image based on compile_target device image property #14909
Conversation
@@ -1,5 +1,6 @@ | |||
set(CMAKE_CXX_EXTENSIONS OFF) | |||
add_sycl_unittest(ProgramManagerTests OBJECT | |||
CompileTarget.cpp |
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.
Shouldn't this be alphabetically sorted? Not sure really, some of the elements don't seem to be.
|
||
// Device image has the compile_target property, so make sure it is equal | ||
// to the Device's architecture. | ||
auto CompileTargetByteArray = DeviceBinaryProperty(*PropIt).asByteArray(); |
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.
There is asCString
helper:
const char *asCString() const; |
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 the property data guaranteed to be null terminated though? From my understanding the memory layout of these property values come from the PropertyValue
class, which I don't think explicitly null terminates the data.
llvm/llvm/include/llvm/Support/PropertySetIO.h
Lines 84 to 86 in cec1423
PropertyValue(const llvm::StringRef &Str) | |
: PropertyValue(reinterpret_cast<const byte *>(Str.data()), | |
Str.size() * sizeof(char) * /* bits in one byte */ 8) {} |
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 the property data guaranteed to be null terminated though?
Good point. I still think that using asCString
would be a better fit, but I assume it requires an update to the helper itself to be made safe first. Therefore, that seems like an improvement that can/should be done separately
queue{}.single_task<NoDeviceKernel>([]() {}); | ||
} catch (sycl::exception &e) { | ||
ASSERT_EQ(e.what(), | ||
std::string("No kernel named NoDeviceKernel was found")); |
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 wonder if this exception message should be improved, because it can be quite confusing. A kernel may be right in front of a developer, but it is not that we couldn't find it, it is our program is not compatible with a device because of how it was compiler.
I suppose that this error message is what we will see even without this patch, so I'm fine with addressing that separately.
Img = getBinImageFromMultiMap(m_KernelIDs2BinImage, KernelId->second, | ||
Context, Device); | ||
assert(Img && "No binary image found for kernel id"); |
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.
Am I right that this assert is effectively replaced by an assert within vector::operator[]
?
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 removed this assert because it is now not guaranteed the Img
will be non null in this case. Specifically, there could be no image compatible with the current device, and getBinImageFromMultiMap
will return nullptr:
https://github.com/intel/llvm/pull/14909/files#diff-78dd7f7ba0b6120dece1ae4ab5a09c9936ff654a1de2c31ff2dbb1fc58d90393R1308-R1309
In this case, the control flow then flows down to the exception. This flow is demonstrated by the NoDeviceKernel
test.
This commit fixes a build failure resulting from intel#14909. Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>
Build issue should be fixed with #15090. |
This commit fixes a build failure resulting from #14909. Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>
When AOT compiling for cpu, the generic `spir64_x86_64` target is used with `-fsycl-targets`. #14909, functionality was added to select device images based on their `compile_target` property in the image. The selection mechanism had to consider CPU as a special case due to not having explicit targets. However, the mechanism only considered `x86_64` and not `intel_cpu_spr` or `intel_cpu_gnr`; therefore on a `intel_cpu_spr` or `intel_cpu_gnr` device, trying to launch a program compiled with `-fsycl-targets=spir64_x86_64`, device image selection would fail to find an image (and thus fail to launch any kernels). This PR updates the logic to include `intel_cpu_spr` and `intel_cpu_gnr`. Note: for tests, this functionality is checked by any test that AOT compiled for CPU and launches a kernel (includes [AOT/cpu.cpp](https://github.com/intel/llvm/blob/sycl/sycl/test-e2e/AOT/cpu.cpp), [AOT/double.cpp](https://github.com/intel/llvm/blob/sycl/sycl/test-e2e/AOT/double.cpp), [AOT/half.cpp](https://github.com/intel/llvm/blob/sycl/sycl/test-e2e/AOT/half.cpp)).
Follow up to #14757