[SYCL] Fix integer type overflow in SYCLDeviceRequirements #10614
Conversation
If reqd_workg_roup_size has max unsigned value then it doesn't fit into signed integer type.
|
@jzc: FYI |
| auto ExtractIntegerFromMDNodeOperand = [=](const MDNode *N, | ||
| unsigned OpNo) -> int32_t { | ||
| auto ExtractSignedIntegerFromMDNodeOperand = [=](const MDNode *N, | ||
| unsigned OpNo) -> int64_t { |
There was a problem hiding this comment.
Do we really need 64-bit integers here?
There was a problem hiding this comment.
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.
| auto ExtractIntegerFromMDNodeOperand = [=](const MDNode *N, | ||
| unsigned OpNo) -> int32_t { | ||
| auto ExtractSignedIntegerFromMDNodeOperand = [=](const MDNode *N, | ||
| unsigned OpNo) -> auto { |
There was a problem hiding this comment.
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.
| 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); |
There was a problem hiding this comment.
Out of curiosity, where do these internal aspects with negative values come from? I am unfamiliar with them
There was a problem hiding this comment.
I added them in scope of this PR: #10140
For example, if reqd_workg_roup_size has max unsigned value then it doesn't fit into signed integer type. So use signed integer type only for sycl_used_aspects which may have negative values.
For example, if reqd_workg_roup_size has max unsigned value then it doesn't fit into signed integer type.
So use signed integer type only for sycl_used_aspects which may have negative values.