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

Look through SSAValue to find include #869

Merged
merged 1 commit into from
Dec 5, 2024
Merged

Look through SSAValue to find include #869

merged 1 commit into from
Dec 5, 2024

Conversation

Keno
Copy link
Collaborator

@Keno Keno commented Dec 5, 2024

There's no guarantee that all include calls will be literal GlobalRefs and in fact JuliaLang/julia#56746 will likely make them never GlobalRef.

There's no guarantee that all include calls will be literal GlobalRefs
and in fact JuliaLang/julia#56746 will likely
make them never GlobalRef.
@Keno Keno force-pushed the kf/ssalookthrough branch from 687c7d5 to a2943da Compare December 5, 2024 01:25
@Keno
Copy link
Collaborator Author

Keno commented Dec 5, 2024

Nightly CI failure will be JuliaLang/julia#56756

Comment on lines +54 to +56
while isa(f, Core.SSAValue) || isa(f, JuliaInterpreter.SSAValue)
f = code[f.id]
end
Copy link
Collaborator

Choose a reason for hiding this comment

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

We have quite a bit of similar code in JuliaInterpreter and LoweredCodeUtils, so it might be a good idea to consolidate it into a single utility.
Other code typically doesn’t use while like this but instead only follows a single level of SSAValue references. I’ll handle the refactoring later.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There's lots of hacks all over the place. I think we need to take a much wider look at refactoring this stack next year, probably once JuliaLowering is ready. It's just all way too ad hoc and fragile

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah overhauling the whole stack at once with integrating JuliaLowering would be very reasonable.

@Keno Keno merged commit d21c32f into master Dec 5, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants