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

inference: allow semi-concrete interpretation on non-inlineable code #46637

Closed
wants to merge 1 commit into from

Conversation

aviatesk
Copy link
Member

@aviatesk aviatesk commented Sep 5, 2022

Currently we require const_prop_methodinstance_heuristic to do the
semi-concrete interpretation, and thus non-inlineable functions are
not eligible for the semi-concrete interpretation.
Especially that can happen for function marked with @noinline.

This commit drops the condition for the semi-concrete interpretation,
and widens the eligibility of it.

Currently we require `const_prop_methodinstance_heuristic` to do the
semi-concrete interpretation, and thus non-inlineable functions are
not eligible for the semi-concrete interpretation.
Especially that can happen for function marked with `@noinline`.

This commit drops the condition for the semi-concrete interpretation,
and widens the eligibility of it.
@aviatesk aviatesk requested a review from Keno September 5, 2022 13:02
@aviatesk
Copy link
Member Author

aviatesk commented Sep 5, 2022

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

@nanosoldier
Copy link
Collaborator

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

@Keno
Copy link
Member

Keno commented Sep 5, 2022

I'm not opposed to it, but I am somewhat worried about it. The restriction to inlineable makes sense because we're gonna uncompress the IR and mess with it. We'll do that anyway for inlining, so there's not a lot lost if it's unprofitable. I'm worried about the performance implications of making this much wider.

@brenhinkeller brenhinkeller added the compiler:inference Type inference label Nov 17, 2022
@aviatesk
Copy link
Member Author

aviatesk commented Aug 2, 2023

Right. We should probably close this PR for now. We may revisit it with a more sophisticated constprop/inlining heuristic as experimented at #46471.

@aviatesk aviatesk closed this Aug 2, 2023
@aviatesk aviatesk deleted the avi/semi-concrete-noinline branch August 2, 2023 11:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler:inference Type inference
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants