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

Trying to support OpaqueClosure in code_llvm/native #41691

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tkf
Copy link
Member

@tkf tkf commented Jul 24, 2021

With this patch, @code_llvm prints something when it sees an opaque closure. But I'm not entirely sure if this is the right way to do it. Is it wrong to recompile opaque closure like this, isolated from the surroundings? Or does it still keep enough information in the method?

@tkf tkf requested a review from Keno July 24, 2021 01:34
@Keno
Copy link
Member

Keno commented Jul 25, 2021

So in theory this is not really a well defined operation. The only thing that OpaqueClosures guarantee is that you can invoke them. For any introspection, we need to decide on semantics. I don't think this is necessarily wrong, but it might mislead people into thinking that this is what actually is going to get executed. In actuality, the optimizer is free to completely rewrite the code if it so pleases. I forget where, but we discussed before that we may want to keep a flag in the opaque closure that indicates whether or not its captures are complete, which would allow us to error out in the misleading cases.

@tkf
Copy link
Member Author

tkf commented Jul 25, 2021

Yes, I remember our discussion on slack. That's why I was unsure about the correctness of this implementation.

Do you think it's worth having something like this? That is to say, do you think we will have a large enough class of opaque closures where this implementation of @code_llvm works? How about after more aggressive optimizations for opaque closures are added in the future? I'm just wondering if a patch like this is worth the trouble.

Also, does the current compiler already have the optimizations that make my implementation incorrect?

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.

None yet

2 participants