[ConstantRange] Fix miscompile caused by off by 1 bugs in UIToFP and SIToFP handling #8452
Conversation
AZero13
changed the title
AZero13
changed the title
AZero13
changed the title
|
It looks like we have the same bug upstream. Any reason not to fix it there and then cherrypick the fix to the relevant Apple branches? |
It's merged upstream now. |
|
@JDevlieghere Now that this code is merged upstream and is being backported to the release branch. |
|
Can you please cherrypick the upstream commit with |
The range for these operations is being constructed without the maximum value for the range due to an incorrect usage of the ConstantRange constructor. This causes Float2Int to think the range for 'uitofp i1' only contains 0 instead of 0 and 1.
…m#86041) We were passing the min and max values of the range to the ConstantRange constructor, but the constructor expects the upper bound to 1 more than the max value so we need to add 1. We also need to use getNonEmpty so that passing 0, 0 to the constructor creates a full range rather than an empty range. And passing smin, smax+1 doesn't cause an assertion. I believe this fixes at least some of the reason llvm#79158 was reverted.
|
@JDevlieghere Is this good? |
|
I don't have perms to do swift ci tests or whatever by the way that's why I ask. |
|
Could you please rebase this on |
Done! |
|
@swift-ci test |
|
@swift-ci test llvm |
|
@swift-ci test windows |
|
@adrian-prantl @JDevlieghere Are these results okay? |
We were passing the min and max values of the range to the ConstantRange
constructor, but the constructor expects the upper bound to 1
more than the max value so we need to add 1.
We also need to use getNonEmpty so that passing 0, 0 to the constructor
creates a full range rather than an empty range. And passing smin, smax+1
doesn't cause an assertion.
This is significant because this fixes a miscompile regarding Float2Int when
the float converts to a very large integer.
(cherry picked from commit llvm@1283646)