-
Notifications
You must be signed in to change notification settings - Fork 6.2k
Create BaseKonanSymbols to reduce symbols for Fir2IR phase #5600
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
Conversation
ivandev0
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make it slightly different. Please move all these required symbols into PreSerializationNativeSymbols and implement them in PreSerializationNativeSymbols.Impl. You can copy/cut required class and callable ids from KonanSymbols. If something is missing (like InteropFqNames) you can add them in NativeStandardInteropNames (with an additional dependency on :core:compiler.common.native insode ir:backend.common). Maybe there is a better approach than copy paste, but it is better to be commented by someone from the Native team (cc @SvyatoslavScherbina )
Copying is fine with me. Anyway, I need to take a look at what values will need to be copied first to have a real opinion. |
5d8d941 to
7120e87
Compare
ivandev0
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change is correct. Just some minor comments.
compiler/cli/cli-native-klib/src/main/kotlin/org/jetbrains/kotlin/native/Fir2Ir.kt
Outdated
Show resolved
Hide resolved
...iler/ir/backend.common/src/org/jetbrains/kotlin/backend/common/ir/PreSerializationSymbols.kt
Outdated
Show resolved
Hide resolved
...iler/ir/backend.common/src/org/jetbrains/kotlin/backend/common/ir/PreSerializationSymbols.kt
Outdated
Show resolved
Hide resolved
...iler/ir/backend.common/src/org/jetbrains/kotlin/backend/common/ir/PreSerializationSymbols.kt
Outdated
Show resolved
Hide resolved
compiler/ir/backend.native/src/org/jetbrains/kotlin/backend/konan/ir/Ir.kt
Show resolved
Hide resolved
core/compiler.common.native/src/org/jetbrains/kotlin/name/NativeStandardInteropNames.kt
Outdated
Show resolved
Hide resolved
ivandev0
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything is fine from my side
|
Please take a look at Also, please, rebase your branch on top of fresh master. |
6f9f8b5
d118394 to
6f9f8b5
Compare
Done! |
| import kotlin.native.internal.ref.releaseExternalRCRef | ||
|
|
||
| @ExportForCppRuntime | ||
| @PublishedApi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Has anything changed in terms of where these symbols can be used (and when corresponding function calls are now emitted)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that this change requires updating the ABI dump for the native stdlib, but I don't really understand why these symbols are public now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As Ivan said, I followed the instructions for fixing the NativePreSerializationTest and this change was suggested in the error logs, and it fixed the test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ivandev0 Shouldn't it be @UsedFromCompilerGeneratedCode instead of @PublishedApi?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't it be @UsedFromCompilerGeneratedCode instead of @PublishedApi?
It is already marked as UsedFromCompilerGeneratedCode, but now we say that this symbol is used on the first compilation stage and so by our rules it requires PublishedApi
Has anything changed in terms of where these symbols can be used (and when corresponding function calls are now emitted)?
No, everything is the same. We just formalized how these symbols can be used.
There will be a change with a new inline mode enabled for klib backends (when inliner works before serialization). We will start to write these symbols into klibs and so all the internal declarations must be marked with PublishedApi.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer these symbols not to become public. Please explore alternatives.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that these symbols are not actually used to create new declarations (at least for the compilation phase before serialization). I think we can simplify some things. Let me check it for a bit and I will come back with a plan.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ivandev0 Did you figure out something or should I proceed with the current changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aviroop-123 Yes, I am working on a fix that will simplify things for you. Please wait a little bit.
ivandev0
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please squash all the changes and edit the commit message to reflect the change properly. Also, please rebase on the latest green master (without it I can't run CI to fully check your changes).
6f9f8b5 to
5200439
Compare
5200439 to
f1884ae
Compare
Done! |
ivandev0
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Please run
org.jetbrains.kotlin.tools.tests.KlibPublicAPITestand fix the fallingnativeStdlibtest. - Rebase on master once again because I see that there are conflicts in your branch.
- Please change the commit message. It does not reflect that was actually changed (we did not create
BaseKonanSymbols).
|
@aviroop-123 I pushed the necessary preparation commits. Please take a look at 767551b. After this commit you don't need Also, I didn't find any reasons to move |
|
@aviroop-123 Closing this PR. I fixed the issue in this commit |
https://youtrack.jetbrains.com/issue/KT-83129