Skip to content

[SYCL] Fix integer type overflow in SYCLDeviceRequirements #10614

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Aug 2, 2023
Merged

[SYCL] Fix integer type overflow in SYCLDeviceRequirements #10614

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
358b149
[SYCL] Fix integer type overflow in SYCLDeviceRequirements
againull Jul 28, 2023
ab6d106
Fix format
againull Jul 28, 2023
546b073
Remove unnecessary static_cast
againull Jul 31, 2023
0b01171
Explicit return types and remove redundant assert
againull Aug 1, 2023
db4285a
Format
againull Aug 1, 2023
35636f5
Merge remote-tracking branch 'origin/sycl' into fix_overflow
againull Aug 2, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 20 additions & 10 deletions llvm/tools/sycl-post-link/SYCLDeviceRequirements.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,18 @@ using namespace llvm;
void llvm::getSYCLDeviceRequirements(
const module_split::ModuleDesc &MD,
std::map<StringRef, util::PropertyValue> &Requirements) {
auto ExtractIntegerFromMDNodeOperand = [=](const MDNode *N,
unsigned OpNo) -> int32_t {
auto ExtractSignedIntegerFromMDNodeOperand = [=](const MDNode *N,
unsigned OpNo) -> int64_t {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really need 64-bit integers here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

getSExtValue() returns int64_t https://llvm.org/doxygen/classllvm_1_1ConstantInt.html#aa8f4be0661aa64f5b1f20b15e93bb403, so that seemed logically correct to me, otherwise we will need cast to smaller type.
Also removed unnecessary static_cast. from both lambdas.

Constant *C =
cast<ConstantAsMetadata>(N->getOperand(OpNo).get())->getValue();
return static_cast<int32_t>(C->getUniqueInteger().getSExtValue());
return C->getUniqueInteger().getSExtValue();
};

auto ExtractUnsignedIntegerFromMDNodeOperand =
[=](const MDNode *N, unsigned OpNo) -> uint64_t {
Constant *C =
cast<ConstantAsMetadata>(N->getOperand(OpNo).get())->getValue();
return C->getUniqueInteger().getZExtValue();
};

// { LLVM-IR metadata name , [SYCL/Device requirements] property name }, see:
Expand All @@ -42,11 +49,15 @@ void llvm::getSYCLDeviceRequirements(
for (const Function &F : MD.getModule()) {
if (const MDNode *MDN = F.getMetadata(MDName)) {
for (size_t I = 0, E = MDN->getNumOperands(); I < E; ++I) {
// Don't put internal aspects (with negative integer value) into the
// requirements, they are used only for device image splitting.
auto Val = ExtractIntegerFromMDNodeOperand(MDN, I);
if (Val >= 0)
Values.insert(Val);
if (std::string(MDName) == "sycl_used_aspects") {
// Don't put internal aspects (with negative integer value) into the
// requirements, they are used only for device image splitting.
auto Val = ExtractSignedIntegerFromMDNodeOperand(MDN, I);
Copy link
Contributor

@jzc jzc Aug 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Out of curiosity, where do these internal aspects with negative values come from? I am unfamiliar with them

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 added them in scope of this PR: #10140

if (Val >= 0)
Values.insert(Val);
} else {
Values.insert(ExtractUnsignedIntegerFromMDNodeOperand(MDN, I));
}
}
}
}
Expand All @@ -69,8 +80,7 @@ void llvm::getSYCLDeviceRequirements(
for (const Function *F : MD.entries()) {
if (auto *MDN = F->getMetadata("intel_reqd_sub_group_size")) {
assert(MDN->getNumOperands() == 1);
auto MDValue = ExtractIntegerFromMDNodeOperand(MDN, 0);
assert(MDValue >= 0);
auto MDValue = ExtractUnsignedIntegerFromMDNodeOperand(MDN, 0);
if (!SubGroupSize)
SubGroupSize = MDValue;
else
Expand Down