-
Notifications
You must be signed in to change notification settings - Fork 1.9k
C++: Divide number of bounds between branches for phi nodes #21329
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
jketema
merged 6 commits into
github:main
from
paldepind:cpp/simple-range-analysis-phi-divide
Feb 20, 2026
+6,532
−6,300
Merged
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
da527ff
C++: Add simple range analysis test with repeated if-else statements
paldepind 032c7ea
C++: Include the actual number of lower/upper bounds for added contex…
paldepind d0681c6
C++: Divide nr of bounds between branches for phi nodes
paldepind fdbd49a
C++: Improve clarity in comment
paldepind 8eed18a
C++: Fix typo
paldepind 9228304
Merge branch 'main' into cpp/simple-range-analysis-phi-divide
jketema File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or 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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My knowledge of the simple range analysis library, so excuse my ignorance.
This I don't understand, and it's not clear how relate this with the division by 2 you do below. Why do all lower bounds flow to
e1and why is the "smaller than" condition there in the case ofe2?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question. Maybe it's easiest to explain by making things a bit more concrete.
Suppose the lower bounds for
xare{2, 11, 22}andcis the constant5.In the true branch we know
x < 5. This is an upper bound and thus doesn't affect the lower bounds, hence the "all lower bounds flow to{ e1 }".In the false branch we know
x >= 5which allow us to prevent the lower bounds 11 and 22 from flowing tox(done here). Sox's bounds will be{2, 5}hence the "only lower bounds that are smaller thancflow to{ e2 }"At the phi node after the if statement the bounds from both branches are joined and we end up with 5 lower bounds:
{2, 5, 11, 22}.It's important to get that final estimate correct, as inaccuracies there can compound (as in the coding standards test with a gazillion if statements after each other) and dividing by 2 and adding a half gets us there.
In general we can't know how many bounds will exist inside each branch. But the number of bounds will be at most the number of bounds on
xand at least 1, so guessing right in the middle reduces how wrong we can be in the worst case.Does that make sense and should I try and expand on the comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the explanation. I think it's fine to leave it as is. I clearly just don't have enough background on how the lower and upper bounds interact.
I'm happy to approve once you've fixed the typo I spotted.