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

cfg_simplify: Avoid ambiguous magic number #52111

Merged
merged 1 commit into from
Nov 11, 2023

Conversation

topolarity
Copy link
Member

This code was using the sentinel value -1 as a special marker in addition to the negative BB indices. That ambiguity was causing cfg_simplify! to fail on functions that merge any BB into the first basic block.

This change is to use typemin(Int) as a marker instead.

Fixes #52058

This code was using the sentinel value -1 as a special marker in
addition to the negative BB indices. That ambiguity was causing
`cfg_simplify!` to fail on functions that merge any BB into the
first basic block.

This change is to use `typemin(Int)` as a marker instead.

Fixes JuliaLang#52058
Copy link
Sponsor Member

@aviatesk aviatesk left a comment

Choose a reason for hiding this comment

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

Thanks!

@vtjnash vtjnash added compiler:optimizer Optimization passes (mostly in base/compiler/ssair/) status:merge me PR is reviewed. Merge when all tests are passing backport 1.10 Change should be backported to the 1.10 release labels Nov 10, 2023
@oscardssmith
Copy link
Member

Before merging this, can you make it so this is a named constant that explains somewhat what it does?

@aviatesk
Copy link
Sponsor Member

IMO this is fine as is, since the uses of typemin(Int) as a special marker are closed within cfg_simplify!. It might be nice to give it a name later if we come up with a good name.

@aviatesk
Copy link
Sponsor Member

The test failure in test i686-w64-mingw32 seems to be real, but unrelated to this PR (it's reproduced on master too). Going to merge.

@aviatesk aviatesk merged commit fcd62da into JuliaLang:master Nov 11, 2023
8 of 10 checks passed
@aviatesk
Copy link
Sponsor Member

This does not need to be backported as cfg_simplify! isn't being used within the compiler implementation for v1.10. If we backport this, we need to backport #51958 to avoid conflicts.

@aviatesk aviatesk removed the backport 1.10 Change should be backported to the 1.10 release label Nov 11, 2023
@giordano giordano removed the status:merge me PR is reviewed. Merge when all tests are passing label Nov 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler:optimizer Optimization passes (mostly in base/compiler/ssair/)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cfg_simplify! error
5 participants