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

adjustments to the latest master #284

Merged
merged 3 commits into from
Mar 29, 2024
Merged

adjustments to the latest master #284

merged 3 commits into from
Mar 29, 2024

Conversation

aviatesk
Copy link
Member

@aviatesk aviatesk commented Mar 18, 2024

Many adjustments...

@aviatesk aviatesk force-pushed the avi/1.12 branch 2 times, most recently from 55a6d44 to 9883872 Compare March 19, 2024 16:48
Comment on lines -9 to -10
# This broke some time between 1.10 and 1.11-DEV.10001
@test isa(sin′′, Core.OpaqueClosure{Tuple{Float64}, Float64}) broken=VERSION>=v"1.11-"
Copy link
Member

Choose a reason for hiding this comment

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

oh yay!

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know too much why these broken test cases have been fixed, but I guess it was probably because the previous (::∂☆recurse)(...) implementation has the serious flaw (i.e. the generator uses function that hasn't been defined at the time when the generator function is defined)?

@aviatesk aviatesk force-pushed the avi/1.12 branch 2 times, most recently from 132f9bb to 6eda0db Compare March 23, 2024 06:44
@aviatesk aviatesk mentioned this pull request Mar 23, 2024
@aviatesk aviatesk force-pushed the avi/1.12 branch 4 times, most recently from 6764904 to 44a4308 Compare March 23, 2024 14:02
@aviatesk aviatesk changed the title wip: adjustments to the latest master adjustments to the latest master Mar 26, 2024
Copy link
Member

@oxinabox oxinabox left a comment

Choose a reason for hiding this comment

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

Pending getting CI working. (Not sure when i will have time to look at that)
This looks good.

if interp !== nothing
cis.inferred = true
@static if VERSION ≥ v"1.12.0-DEV.15"
rettype = Any # ci.rettype # TODO revisit
Copy link
Member

Choose a reason for hiding this comment

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

Why? is inference broken?

Copy link
Member Author

Choose a reason for hiding this comment

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

ci no longer has rettype.

Copy link
Member Author

Choose a reason for hiding this comment

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

We probably need further tweak on this code, but maybe later.

else
ci.inferred = true
rettype = ci.rettype
end
ocm = ccall(:jl_new_opaque_closure_from_code_info, Any, (Any, Any, Any, Any, Any, Cint, Any, Cint, Cint, Any),
Copy link
Collaborator

Choose a reason for hiding this comment

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

The signature of this c function changed

Copy link
Collaborator

Choose a reason for hiding this comment

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

That said, this should ideally use the Core.OpaqueClosure constructor

@Keno Keno self-assigned this Mar 27, 2024
Copy link

codecov bot commented Mar 27, 2024

Codecov Report

Attention: Patch coverage is 80.72289% with 32 lines in your changes are missing coverage. Please review.

Project coverage is 54.83%. Comparing base (6c9048c) to head (058aa57).
Report is 60 commits behind head on main.

Files Patch % Lines
src/stage2/interpreter.jl 47.36% 10 Missing ⚠️
src/codegen/reverse.jl 43.75% 9 Missing ⚠️
src/stage1/recurse_fwd.jl 92.30% 7 Missing ⚠️
src/jet.jl 70.00% 3 Missing ⚠️
src/stage1/compiler_utils.jl 80.00% 2 Missing ⚠️
src/stage1/recurse.jl 90.90% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #284      +/-   ##
==========================================
- Coverage   55.29%   54.83%   -0.46%     
==========================================
  Files          28       27       -1     
  Lines        2917     2945      +28     
==========================================
+ Hits         1613     1615       +2     
- Misses       1304     1330      +26     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@aviatesk
Copy link
Member Author

I will give them reviews quickly.

@aviatesk
Copy link
Member Author

It's really nice to have tests passing on the stable version!

@@ -6,36 +6,34 @@ module stage2_fwd
@test sin′(1.0) == cos(1.0)
end
let sin′′ = Diffractor.dontuse_nth_order_forward_stage2(Tuple{typeof(mysin), Float64}, 2)
# This broke some time between 1.10 and 1.11-DEV.10001
@test isa(sin′′, Core.OpaqueClosure{Tuple{Float64}, Float64}) broken=VERSION>=v"1.11-"
Copy link
Member Author

Choose a reason for hiding this comment

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

These test cases about inferred OC types appear to work fine in v1.12, but they're still broken on v1.11. We might need to consider backporting some changes, or tweak the broken condition here.

@aviatesk aviatesk merged commit 374f92b into main Mar 29, 2024
9 of 11 checks passed
@aviatesk aviatesk deleted the avi/1.12 branch March 29, 2024 01:12
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.

3 participants