-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
DCE bug: unsafe elimination of dead statement, which might not terminate #52991
Comments
This commit serves as a preliminary PR for #52991. It involves adding `IR_FLAG_TERMINATES` to the optimization flags and carrying out various renaming refactors. As `IR_FLAG_TERMINATES` isn't utilized in this particular commit, it doesn't bring any changes to the compiler's functionality. The incoming commit will make use of it and fix #52991 along with other accompanying changes.
Technically this is not a bug, because we currently still have forward progress guarantees, since we didn't decide on #40009, but I agree, it'd be good to fix this anyway. |
I added #40009 to the triage label so we can agree that this is a bug. |
Sure. The termination modeling was added specifically to be able to make that change, so we might as well take an official call on it. We'll have to see if we need to improve it though once @aviatesk puts together the change. |
Submitted a PR: #52999. |
This commit serves as a preliminary PR for #52991. It involves adding `IR_FLAG_TERMINATES` to the optimization flags and carrying out various renaming refactors. As `IR_FLAG_TERMINATES` isn't utilized in this particular commit, it doesn't bring any changes to the compiler's functionality. The incoming commit will make use of it and fix #52991 along with other accompanying changes.
Upon further investigation, it appears that modifying this single line is enough to remove the call to the dead
PersistentDict
constructor. But, this seems to stem from an inference bug, where we're deleting statements without actually provingterminates
.So, I'd like to address this bug first and then revisit this PR.
Originally posted by @aviatesk in #52954 (comment)
The text was updated successfully, but these errors were encountered: