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

Feat: up plus & minus #111

Merged
merged 4 commits into from
Nov 14, 2023
Merged

Feat: up plus & minus #111

merged 4 commits into from
Nov 14, 2023

Conversation

Shi-Raida
Copy link
Contributor

Add support for up:plus and up:minus operators.

Converts IntCst from i32 into i64 in order to fix overflow issues on LinearSumLeq propagation. The bounds of the sum's terms are clamped into the set_ub method.

NOTE: It was not possible to have INT_CST_MIN = i64::from(i32::MIN) - 1 (same for INT_CST_MAX) since the result is not a constant anymore. Therefore, I hard coded the value of the i32::MIN constant.

@Shi-Raida Shi-Raida requested a review from arbimo November 14, 2023 09:21
@Shi-Raida Shi-Raida self-assigned this Nov 14, 2023
@Shi-Raida Shi-Raida added enhancement New feature or request rust Pull requests that update Rust code labels Nov 14, 2023
@arbimo
Copy link
Member

arbimo commented Nov 14, 2023

@Shi-Raida It looks for the first part but the change to use i64 should only be internal to the linear propagator.
So domains should still be represented with 32 bits (IntCst does not change) but your local copy should be 64 bits. When updating the domains, the 64bits numerals should be clamped and converted to i32.

As implemented, this would have drastic performance implications as the size of a literal would be 128bits (instead of 64), which means it cannot be manipulated as a single machine word. It would also ~double the memory footprint of the solver and negatively impact cache behavior.

@Shi-Raida
Copy link
Contributor Author

@Shi-Raida It looks for the first part but the change to use i64 should only be internal to the linear propagator. So domains should still be represented with 32 bits (IntCst does not change) but your local copy should be 64 bits. When updating the domains, the 64bits numerals should be clamped and converted to i32.

As implemented, this would have drastic performance implications as the size of a literal would be 128bits (instead of 64), which means it cannot be manipulated as a single machine word. It would also ~double the memory footprint of the solver and negatively impact cache behavior.

Everything should be fine now.

@arbimo arbimo merged commit 69e58c8 into master Nov 14, 2023
10 checks passed
@Shi-Raida Shi-Raida deleted the feat/up-plus branch November 15, 2023 06:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request rust Pull requests that update Rust code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants