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

Fixes for Julia nightly #300

Open
wants to merge 19 commits into
base: main
Choose a base branch
from
Open

Conversation

serenity4
Copy link

@serenity4 serenity4 commented Feb 21, 2025

Makes Diffractor functional on nightly, ensuring that the test suite runs without errors.

Requires JuliaDiff/ChainRules.jl#819 merged and JuliaDebug/Cthulhu.jl#621 released to be fully functional.

Before merge (marking as draft until completed, but this is ready for review):

A significant refactor of the overload of CC.finishinfer!/CC.finish!/CC.transform_result_for_cache was made to follow exactly the same logic as Cthulhu (these seem to have been copy-pasted from there into Diffractor, the changes simply sync them).

The most notable change in this PR is that it works around IR changes, as described below.

GlobalRef changes

On nightly, what used to be a GlobalRef in lowered IR may now be a getproperty instead, with a first Module SSA argument and a symbol. To illustrate this, instead of something like

%1 = B.x

we now have

%1 = Main.B
%2 = getproperty(%1, :x)

As a possible fix, we may preserve the old semantics by associating the latter IR with the former. This is what I attempted to fix the failing tests, but as the IR transforms are performed within a generated function we cannot evaluate GlobalRefs for module arguments (as in Main.B) to know whether getproperty is a global access or not (i.e. is the first argument a module), because we can't see past the world age of the generated function definition.

Instead, we instrument the getproperty call for differentiation, and I added a rrule with NoTangent().

@serenity4
Copy link
Author

serenity4 commented Feb 21, 2025

This will also need a proper Compiler versioning. I went with deving the path in my Julia nightly installation, perhaps we can revert to using Base.Compiler instead of relying on the package to keep things in sync.

EDIT: this is discussed at JuliaLang/julia#57520

Copy link
Collaborator

@Keno Keno left a comment

Choose a reason for hiding this comment

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

Overall LGTM

@serenity4 serenity4 marked this pull request as ready for review February 28, 2025 21:59
@serenity4
Copy link
Author

The compiler versioning issue set aside, this is in a working state and can be merged. Perhaps best to get this in now so we may move forward with DAECompiler, even if it requires deving the right Compiler path locally.

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