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

DCE bug: unsafe elimination of dead statement, which might not terminate #52991

Closed
aviatesk opened this issue Jan 21, 2024 · 4 comments · Fixed by #52998 or #52999
Closed

DCE bug: unsafe elimination of dead statement, which might not terminate #52991

aviatesk opened this issue Jan 21, 2024 · 4 comments · Fixed by #52998 or #52999
Assignees

Comments

@aviatesk
Copy link
Sponsor Member

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 proving terminates.

julia> @noinline Base.@assume_effects :effect_free :nothrow function foo(n)
           s = 0
           while true
               if n - rand(1:10) > 0
                   s += 1
               else
                   break
               end
           end
           return s
       end^C

julia> code_typed((Int,)) do n
           foo(n)
           nothing
       end
1-element Vector{Any}:
 CodeInfo(
1return Main.nothing
) => Nothing

So, I'd like to address this bug first and then revisit this PR.

Originally posted by @aviatesk in #52954 (comment)

@aviatesk aviatesk self-assigned this Jan 21, 2024
aviatesk added a commit that referenced this issue Jan 22, 2024
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.
@Keno
Copy link
Member

Keno commented Jan 22, 2024

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.

@oscardssmith
Copy link
Member

I added #40009 to the triage label so we can agree that this is a bug.

@Keno
Copy link
Member

Keno commented Jan 22, 2024

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.

@aviatesk
Copy link
Sponsor Member Author

Submitted a PR: #52999.

aviatesk added a commit that referenced this issue Jan 22, 2024
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.
@Keno Keno reopened this Jan 22, 2024
aviatesk added a commit that referenced this issue Apr 15, 2024
Fixes #52991.
Currently this commit marks the test case added in #52954 as `broken`
since it has relied on the behavior of #52991.
I'm planning to add followup changes in a separate commit.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants