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

minor followups on recent CodeInstance refactors #53581

Merged
merged 1 commit into from
Mar 21, 2024

Conversation

aviatesk
Copy link
Sponsor Member

@aviatesk aviatesk commented Mar 4, 2024

  • simplifies the signature of transform_result_for_cache
  • make jl_uncompress_ir take MethodInstance instead of CodeInstance
    and simplifies the inlining algorithm
  • renames of codeinst::CodeInstace objects
  • removal of dead code

@aviatesk aviatesk requested review from vtjnash and Keno March 4, 2024 01:14
@aviatesk aviatesk force-pushed the avi/follow-up-codeinst-refactor branch from f8d7b0f to fb28d67 Compare March 4, 2024 03:40
vtjnash
vtjnash previously requested changes Mar 4, 2024
Copy link
Sponsor Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

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

The metadata argument is mandatory to be a CodeInstance, as it contains some of the required context for uncompressing and populating the CodeInfo correctly and efficiently

Comment on lines 344 to 337
function transform_result_for_cache(interp::AbstractInterpreter,
linfo::MethodInstance, valid_worlds::WorldRange, result::InferenceResult,
can_discard_trees::Bool=may_discard_trees(interp))
function transform_result_for_cache(interp::AbstractInterpreter, result::InferenceResult)
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

This is fine to change, but it had seemed inconvenient to cause breakage for external clients which are supposed to overload this method signature.

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

Well, that does happen frequently. The overload signature of transform_result_for_cache was changed in #53219, and this PR serves as a follow-up to that change. So I believe making modifications at this PR wouldn't be an issue. We need to add some updates to external abstract interpreter packages anyway.

Base automatically changed from avi/noopt-interp-irinterp to master March 4, 2024 17:58
@aviatesk
Copy link
Sponsor Member Author

aviatesk commented Mar 5, 2024

The metadata argument is mandatory to be a CodeInstance, as it contains some of the required context for uncompressing and populating the CodeInfo correctly and efficiently

Is it not feasible to check the identity of codeinst.def and mi in the callers of _uncompress_ir(mi, src)? I'd prefer to avoid the current state of affair where external abstract interpreter needs to have two overloads of retrieve_ir_for_inlining.

@aviatesk aviatesk force-pushed the avi/follow-up-codeinst-refactor branch from fb28d67 to 5c8e5d3 Compare March 5, 2024 03:59
@aviatesk aviatesk force-pushed the avi/follow-up-codeinst-refactor branch from 5c8e5d3 to 772fd06 Compare March 18, 2024 15:24
@aviatesk
Copy link
Sponsor Member Author

Having looked over #52415, I've gotten why jl_uncompress_ir requires a metadata::CodeInstance. That said, the custom inlining pass interface of src_inlining_policy and retrieve_ir_for_inlining needs more work, but I'm thinking of tackling that in another PR. This one keeps things pretty much cosmetic changes here, without messing with the compiler's workings too much.

@aviatesk
Copy link
Sponsor Member Author

@nanosoldier runbenchmarks("inference", vs=":master")

@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - no performance regressions were detected. A full report can be found here.

base/reflection.jl Outdated Show resolved Hide resolved
@aviatesk aviatesk force-pushed the avi/follow-up-codeinst-refactor branch from 772fd06 to fd81aa4 Compare March 19, 2024 05:41
@aviatesk
Copy link
Sponsor Member Author

I'd like to move this forward. @vtjnash can you give another review on this?

Copy link
Sponsor Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

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

I am on vacation this month, so proceed as you see fit. I will try to remove my "request changes" if any

@aviatesk aviatesk dismissed vtjnash’s stale review March 21, 2024 03:13

Changes have been addressed.

@aviatesk
Copy link
Sponsor Member Author

I dismissed it from my side. Have a great vacation!

@aviatesk aviatesk merged commit 8e8b533 into master Mar 21, 2024
6 of 8 checks passed
@aviatesk aviatesk deleted the avi/follow-up-codeinst-refactor branch March 21, 2024 03:14
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