Skip to content
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.

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

Add fixes to overflow analysis in bounds inference #5618

Merged
merged 20 commits into from
Feb 4, 2021

Conversation

rootjalex
Copy link
Member

Add fixes (and tests) to bounds inference for the following cases:

  • UInt(32) addition and multiplication overflow was not properly detected.
  • Division of type.min() by -1 for Int(16) and Int(8) was not detected as overflow.
  • Ramps of types that cannot represent their lanes parameter would produce incorrect bounds (e.g. a ramp of bools)

@rootjalex rootjalex changed the title add fixes to overflow analysis in bounds inference Add fixes to overflow analysis in bounds inference Jan 7, 2021
@rootjalex rootjalex added the bug label Jan 8, 2021
src/Bounds.cpp Outdated Show resolved Hide resolved
src/Bounds.cpp Outdated Show resolved Hide resolved
src/Bounds.cpp Outdated Show resolved Hide resolved
@alexreinking alexreinking added this to the v12.0.0 milestone Jan 8, 2021
@rootjalex
Copy link
Member Author

@abadams Fixed your comments and also added can_overflow and can_overflow_int functions to Type, I imagine it'll be easier to change things if overflow behavior for different types is changed.

@steven-johnson
Copy link
Contributor

I can open an issue to track this, if deemed worth it!

I'd argue that, as a rule of thumb, all TODOs that are checked in to the codebase should generally have a tracking issue (referenced in the comment). Not always, but usually.

@rootjalex
Copy link
Member Author

rootjalex commented Feb 1, 2021

Sorry for the delay @steven-johnson . I created #5682 for this problem, and updated the TODOs in 6fa0a56 .

Copy link
Contributor

@steven-johnson steven-johnson left a comment

Choose a reason for hiding this comment

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

LGTM with nits.

src/Bounds.cpp Outdated Show resolved Hide resolved
src/Bounds.cpp Show resolved Hide resolved
@steven-johnson
Copy link
Contributor

@abadams is this ready to land?

src/Type.h Outdated Show resolved Hide resolved
src/Type.h Outdated Show resolved Hide resolved
Copy link
Member

@abadams abadams left a comment

Choose a reason for hiding this comment

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

lgtm with two comment nits

@rootjalex rootjalex merged commit a0fc7fa into master Feb 4, 2021
@rootjalex rootjalex deleted the rootjalex/bounds_overflow_fixes branch February 4, 2021 01:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants