-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
C++: Fix forex
recursion in sign analysis
#18310
C++: Fix forex
recursion in sign analysis
#18310
Conversation
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.
Copilot encountered an error and was unable to review this pull request. You can try again, by re-requesting a review.
DCA looks good. Zero result changes (as expected), and now the problematic project no longer times out 🎉 |
/** | ||
* Gets a possible sign of `v` at read position `pos`, where a guard could have | ||
* ruled out the sign but does not. | ||
* This does not check that the definition of `v` also allows the sign. | ||
*/ | ||
private Sign guardedSsaSignOk(SemSsaVariable v, SsaReadPosition pos) { | ||
result = TPos() and | ||
forex(SemExpr bound | posBound(bound, v, pos) | posBoundOk(bound, v, pos)) | ||
posBoundGuardedSsaSignOk(v, pos) |
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.
Let's leave in the original line as a comment:
posBoundGuardedSsaSignOk(v, pos) | |
// optimised version of | |
// `forex(SemExpr bound | posBound(bound, v, pos) | posBoundOk(bound, v, pos))` | |
posBoundGuardedSsaSignOk(v, pos) |
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.
Same for the others.
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.
Fixed in 4ffe70d
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.
It's a shame that it's necessary, but LGTM.
It's well-known that recursion through
forall
/forex
may perform poorly. Apparently we do recursion throughforex
in the sign analysis library (which is part of the new range analysis library for C++). This results in some unfortunate tuple counts because the "range" part of theforall
/forex
gets joined with itself:Luckily, there's a standard way to work around this problem by rewriting the
forall
/forex
to recursion over integers. This PR does just that. This results in much better RA.