Fix possible overflow in saturating_cast bounds inference #6961
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Testing now. (We should add the failure case as a test when we land this, of course.) |
Thanks!
Ah yes, I will make that a test now. |
|
Things simplifying to overflow in bounds seems like a more general problem than saturating casts. Shouldn't we instead check for signed integer overflow at the output of bounds analysis and set it to be unbounded? |
|
Yeah, this fixes some but definitely not all of the cases I'm seeing. Should I try to find another (different) repro case? |
@steven-johnson yes please!
@abadams I think the best way to do so while not reducing the current power of bounds inference would be to insert code at every recursive call that calls |
|
I think that might be necessary. A helper function that simplifies the bounds on an interval and replaces signed integer overflow with undefined Exprs might be the thing to do. |
|
E.g. in the cast visitor we need to simplify the bounds on |
|
As I mentioned on #6900, with this patch in place, there are still at least two constant-fold-overflow failures inside google code; unfortunately, these are depended on by hundreds of projects, and it's highly unlikely I'll be able to extract repro cases to share from them without herculean effort (one is totally off-limits, the other is an extremely complex pipeline that has so far resisted the ability to simplify it even a little without it "healing" itself). So either it's gonna take some careful thinking to track this down, or maybe some additional debugging code inserted to try to track down pathological situations more quickly. |
|
@abadams I think we need to simplify with every recursive call, as most operations can produce overflow. I plan to rewrite all recursive calls into a helper used like (assuming where |
|
@steven-johnson do we need to revert #6900 for now then? |
|
I just added a fix for the OpenGL issue @steven-johnson mentioned on #6900 . I believe all other non-CPU backends fall back to |
Do you think your latest changes may have addressed the issue? If so, I'll apply them downstream and retry. (I don't think we need to revert if a fix may be soon, but as a matter of policy, I don't think we should keep known-broken changes in the main branch for any extended length of time) |
3 tasks
Yes, the recent changes should have removed the ability to produce |
|
Unfortunately, no, at least one of the failures still remains ( |
The traceback of the main failure I see now is something like |
|
For the failure that you see, could you enable |
|
Here's a complete backtrace, using a debug build that skips |
Unfortunately, no: |
|
I suspect that the overflow is introduced in bounds inference but is not caught until codegen - can you search in the super large output file for |
Yes, I see |
Dang. Okay, I will search for more possible places it could be introduced. |
|
It appears that signed_integer_overflow was actually being introduced before #6900 , but the lowered form of saturating_cast combined with the very complicated handler for |
|
closing this in favor of #6970 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The issue @steven-johnson noted on #6900 is a result of bounds inference producing integer overflow in the calculation of func value bounds. This PR should fix that issue.