Skip to content

C++: Add support for constant bound modulus operations #12655

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

Merged
merged 3 commits into from
Mar 28, 2023

Conversation

jketema
Copy link
Contributor

@jketema jketema commented Mar 24, 2023

No description provided.

@jketema jketema requested a review from a team as a code owner March 24, 2023 10:52
@github-actions github-actions bot added the C++ label Mar 24, 2023
@jketema jketema force-pushed the range-rem branch 2 times, most recently from a71f5a6 to de22182 Compare March 24, 2023 11:09
Copy link
Contributor

@MathiasVP MathiasVP left a comment

Choose a reason for hiding this comment

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

This seems very sensible, but I'm wondering if it makes more sense to add this case in boundFlowStep (and call bounded recursively on the right-hand side)? I guess this would also handle a case such as:

int n = b ? 10 : 20;
range(unknown() % n)

?

@jketema
Copy link
Contributor Author

jketema commented Mar 24, 2023

This seems very sensible, but I'm wondering if it makes more sense to add this case in boundFlowStep (and call bounded recursively on the right-hand side)?

I'm not sure how to do this. How would this interact with a bound on the left-hand side? I'm not sure how to check whether that bound is missing or not. I started to run into non-monotonic recursion.

@jketema
Copy link
Contributor Author

jketema commented Mar 24, 2023

Or should I just completely ignore any bound coming from the left-hand side?

@MathiasVP
Copy link
Contributor

MathiasVP commented Mar 24, 2023

Or should I just completely ignore any bound coming from the left-hand side?

I think so, yes. I think that's what SimpleRangeAnalysis is also doing.

@jketema
Copy link
Contributor Author

jketema commented Mar 24, 2023

Move to bounded as discussed. Also started a DCA experiment on top of the feature branch.

@MathiasVP
Copy link
Contributor

MathiasVP commented Mar 24, 2023

I just noticed that boundFlowStep already has a case that handles SemRemExpr when the right-hand side is guaranteed to be positive. We might need to exclude that situation in our newly added case to prevent a duplication of computed bounds.

@jketema
Copy link
Contributor Author

jketema commented Mar 27, 2023

I just noticed that boundFlowStep already has a case that handles SemRemExpr when the right-hand side is guaranteed to be positive. We might need to exclude that situation in our newly added case to prevent a duplication of computed bounds.

I'm not sure how. I also having a hard time believing those bound by the way. It's not immediately clear to me why they would be correct.

@MathiasVP
Copy link
Contributor

MathiasVP commented Mar 27, 2023

I'm not sure how.

Since one of the two boundFlowStep cases require that the right-hand side is always positive, you could exclude this case by requiring that not semPositive(rem.getRightOperand()), and the other case could similarly be exclude by requiring that not semPositive(rem.getLeftOperand()).

I also having a hard time believing those bound by the way. It's not immediately clear to me why they would be correct.

Hm, I don't see what's wrong with those bounds. If e = x % y then the first of the two cases in boundFlowStep says that if y <= B + k (for some SemBound B and constant k) then e <= B + (k - 1) (because it transfers the bound computed for y to the bound computed for e with a delta of -1). And the second bounds says that, if x <= B + k then e <= B + k (because it transfers the bound computed for x to the bound computed for e wit ha delta of 0). Aren't both of those bounds correct?

@jketema
Copy link
Contributor Author

jketema commented Mar 27, 2023

Ah. I don't think I was reading the code correctly. I'll have another look.

@jketema
Copy link
Contributor Author

jketema commented Mar 28, 2023

@MathiasVP Updated the PR and ran a new DCA experiment (which looks ok to me).

Copy link
Contributor

@MathiasVP MathiasVP left a comment

Choose a reason for hiding this comment

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

LGTM!

@MathiasVP MathiasVP merged commit 58c7148 into github:main Mar 28, 2023
@jketema jketema deleted the range-rem branch March 28, 2023 07:01
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.

2 participants