-
-
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
fix bug in block renaming for dead code elimination #32145
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.
LGTM, but could ideally use a test.
Thanks @yhls, very nice first PR to julialang/julia. Welcome! |
Thanks for the welcome! I'm going travelling after graduation tomorrow until mid-August, but I'll try to tie this PR up in the next week. I've also found a small bug in my PR, which I'll fix very soon. |
I've just added a test--how do things look? |
Thanks! Can you squash and rebase onto master? |
Ok, squashed and rebased. By the way, I've noticed that with these changes, reversing the change in #29265 (so making IncrementalCompact remove dead code by default again) causes the build process to fail when producing corecompiler.ji (see below). Do you have any suspects in mind for why this might be happening?
|
That looks like an assertion failure. Running |
bump, would be great to have it in 1.3, but it needs another rebase. |
Just rebased. Are we also waiting for me to figure out what goes wrong when DCE in IncrementalCompact is turned on by default again? (I'm still travelling, and won't be able to investigate for a while.) |
Thanks! This seems more correct in general and we can figure out the other bug later |
IncrementalCompact can't deal with predecessors referred to by index 0, but according to @Keno, these are virtual and should not be renamed. I've changed IncrementalCompact to leave these predecessors as they are. To do this, I've also made it so that blocks that are to be removed are renamed to -1 rather than 0, to avoid conflicting with 0s that should be kept.
This should address JuliaDebug/Cthulhu.jl#28