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
againull merged 6 commits into from
Aug 2, 2023
Merged

[SYCL] Fix integer type overflow in SYCLDeviceRequirements #10614

Changes from 3 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
29 changes: 20 additions & 9 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) -> auto {
Copy link
Contributor

@AlexeySachkov AlexeySachkov Aug 1, 2023

Choose a reason for hiding this comment

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

I would prefer to list a concrete return type instead of -> auto here, because it affects readability of the code. Coding standards:

Use auto if and only if it makes the code more readable or easier to maintain. Don’t “almost always” use auto

In this particular case, I don't think that the return type is obvious from a context.

Copy link
Contributor Author

@againull againull Aug 1, 2023

Choose a reason for hiding this comment

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

Indeed, fixed.

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) -> auto {
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

@againull againull Aug 2, 2023

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,7 +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);
auto MDValue = ExtractUnsignedIntegerFromMDNodeOperand(MDN, 0);
assert(MDValue >= 0);
if (!SubGroupSize)
SubGroupSize = MDValue;
Expand Down