[M] Change in dart/sdk[main]: [record_use] Enum const instances wasm
Daco Harkes (Gerrit)
New activity on the change![]( "Open in Gerrit")
Related details
- Nate Biggs
Code-Owners
Code-Review
Commit-Message-Has-TEST
Core-Library-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nate Biggs (Gerrit)
Nate Biggs added 2 comments![]( "Open in Gerrit")
node.isEnum = false;Why do we need to preserve the enum-ness of an unused class?
// TODO(https://github.com/dart-lang/native/issues/3186): Don't record indices.If anything I'd lean towards only putting the index and not the name. It's less readable but most of the time the index is considered the "value of truth" for enums.
Related details
- Daco Harkes
Code-Owners
Code-Review
Commit-Message-Has-TEST
Core-Library-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daco Harkes (Gerrit)
Daco Harkes voted and added 2 comments![]( "Open in Gerrit")
Votes added by Daco Harkes
| Commit-Queue | +1 |
2 comments
Why do we need to preserve the enum-ness of an unused class?
Oh, we don't. 👍
// TODO(https://github.com/dart-lang/native/issues/3186): Don't record indices.If anything I'd lean towards only putting the index and not the name. It's less readable but most of the time the index is considered the "value of truth" for enums.
Let's have this discussion on https://github.com/dart-lang/native/issues/3186.
Related details
- Nate Biggs
Code-Owners
Code-Review
Commit-Message-Has-TEST
Core-Library-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nate Biggs (Gerrit)
Nate Biggs added 1 comment![]( "Open in Gerrit")
if (options.recordedUsesFile != null) {The `Translator` shouldn't be modifying the kernel so I don't really get what moving this before code generation does.
Related details
- Daco Harkes
Code-Owners
Code-Review
Commit-Message-Has-TEST
Core-Library-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daco Harkes (Gerrit)
Daco Harkes added 1 comment![]( "Open in Gerrit")
The `Translator` shouldn't be modifying the kernel so I don't really get what moving this before code generation does.
Right. My reasoning was that if it doesn't modify anything, we should not rely on it either.
I've undone the changes, as they were not necessary.
Related details
- Nate Biggs
Code-Owners
Code-Review
Commit-Message-Has-TEST
Core-Library-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daco Harkes (Gerrit)
Daco Harkes added 1 comment![]( "Open in Gerrit")
node.isEnum = false;Daco HarkesWhy do we need to preserve the enum-ness of an unused class?
Oh, we don't. 👍
Actually, we do 😄
Without this we start reporting the kind as class instead of enum in the record_use file for static methods nested in the enum.
I'll add it again to the CL where I add a test case that uncovered this.
Related details
- Nate Biggs
Code-Owners
Code-Review
Commit-Message-Has-TEST
Core-Library-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nate Biggs (Gerrit)
Nate Biggs voted Code-Review+1![]( "Open in Gerrit")
- Daco Harkes
Code-Owners
Code-Review
Commit-Message-Has-TEST
Core-Library-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daco Harkes (Gerrit)
Daco Harkes voted Commit-Queue+2![]( "Open in Gerrit")
Code-Owners
Code-Review
Commit-Message-Has-TEST
Core-Library-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Commit Queue (Gerrit)
Commit Queue submitted the change![]( "Open in Gerrit")
Change information
[record_use] Enum const instances wasm
Dart2wasm has a bunch of optmizations/lowerings that the VM doesn't
have for enum instances.
Some metadata that we don't want to lose for enum const instance:
* Don't tree-shake any instance fields of classes and enhanced
enums annotated with `@RecordUse()`. (TFA)
* Don't let the `index` and `_name` fields be tree-shaken away (TFA).
(Achieved by adding pragma.)
Closes: https://github.com/dart-lang/native/issues/2908
TEST=pkg/compiler/test/record_use/record_use_test.dart
TEST=pkg/dart2wasm/test/record_use_test.dart
TEST=pkg/vm/test/transformations/record_use_test.dart
- M pkg/dart2wasm/test/ir_tests/deferred.constant.type_use.wat
- M pkg/dart2wasm/test/ir_tests/deferred.constant_module1.wat
- M pkg/dart2wasm/test/ir_tests/deferred.constant_module2.wat
- M pkg/dart2wasm/test/ir_tests/deferred.fine_grained.devirtualized_module1.wat
- M pkg/dart2wasm/test/ir_tests/deferred.fine_grained_module5.wat
- M pkg/dart2wasm/test/ir_tests/deferred.type_checks_module1.wat
- M pkg/dart2wasm/test/ir_tests/dyn_closure.wat
- M pkg/dart2wasm/test/ir_tests/dyn_closure_function_apply.wat
- M pkg/dart2wasm/test/ir_tests/dyn_closure_function_apply_named.wat
- M pkg/dart2wasm/test/ir_tests/dynamic_call.wat
- M pkg/dart2wasm/test/ir_tests/hello.wat
- M pkg/dart2wasm/test/ir_tests/import_name_module1.wat
- M pkg/dart2wasm/test/ir_tests/memory_use.wat
- M pkg/dart2wasm/test/ir_tests/pure_function.wat
- M pkg/dart2wasm/test/ir_tests/try_blocks.wat
- M pkg/dart2wasm/test/record_use_test.dart
- M pkg/vm/lib/transformations/type_flow/transformer.dart
- M sdk/lib/core/enum.dart
Code-Review: +1 by Nate Biggs
Core-Library-Review: Code-Review+1 by Nate Biggs
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Martin Kustermann (Gerrit)
Martin Kustermann added 1 comment![]( "Open in Gerrit")
@pragma('wasm:entry-point')Could you explain why we would add entrypoints for wasm here?
What may happen is that these fields get tree shaken and they may rightfully be so if they are not used.
Users have to mark fields they want to reflect on with some record use annotation and they should only reflect on those fields. Code they cannot control (like core libs or other packages) they should not reflect on.
This is by design: For example I think it's perfectly fine for us to e.g. rename `_name` to `_enumName` here. It shouldn't impact or break anyone.
If you want link hooks to reflect on these fields, then we have to commit on making those fields always be there, never renamed, etc. In that case those two fields here should have a `@RecordUse` annotation and not a `@pragma('wasm:entry-point')`.
In any case, I don't see why handling of these fields should have any wasm specific treatments.
Also: Could you please add me as reviewer when changing dart2wasm related things (as I'm probably the most knowledgeable person for that) and alexmarkov when touching TFA (as he's the most knowledgeable person for that)?
Related details
- Daco Harkes
Code-Owners
Code-Review
Commit-Message-Has-TEST
Core-Library-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daco Harkes (Gerrit)
Daco Harkes added 1 comment![]( "Open in Gerrit")
@pragma('wasm:entry-point')Could you explain why we would add entrypoints for wasm here?
What may happen is that these fields get tree shaken and they may rightfully be so if they are not used.
Users have to mark fields they want to reflect on with some record use annotation and they should only reflect on those fields. Code they cannot control (like core libs or other packages) they should not reflect on.
This is by design: For example I think it's perfectly fine for us to e.g. rename `_name` to `_enumName` here. It shouldn't impact or break anyone.
If you want link hooks to reflect on these fields, then we have to commit on making those fields always be there, never renamed, etc. In that case those two fields here should have a `@RecordUse` annotation and not a `@pragma('wasm:entry-point')`.
In any case, I don't see why handling of these fields should have any wasm specific treatments.
Also: Could you please add me as reviewer when changing dart2wasm related things (as I'm probably the most knowledgeable person for that) and alexmarkov when touching TFA (as he's the most knowledgeable person for that)?
I tried to reconstruct the index and name in another way in dart2wasm but wasn't able to find the right information in the ast at the point where the recordings are done.
(We explored an approach where we do all the recordings in the AST super early before any optimizations and TFA in the metadata repository and then trim it down later after TFA, but that also became very messy. So we went back to the approach where we record after TFA and reconstruct information that is lowered.)
Is there a way to get the name and index in dart2wasm without these entry-points?
Also: Could you please add me as reviewer when changing dart2wasm related things (as I'm probably the most knowledgeable person for that) and alexmarkov when touching TFA (as he's the most knowledgeable person for that)?
Yes, I'm chatting with Alex. We should probably have a chat as well about the high level approach things for dart2wasm where they don't align with the VM (which they don't in a bunch of cases). I'll shoot you a calendar invite.
Related details
Code-Owners
Code-Review
Commit-Message-Has-TEST
Core-Library-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Martin Kustermann (Gerrit)
Martin Kustermann added 1 comment![]( "Open in Gerrit")
@pragma('wasm:entry-point')Daco HarkesCould you explain why we would add entrypoints for wasm here?
What may happen is that these fields get tree shaken and they may rightfully be so if they are not used.
Users have to mark fields they want to reflect on with some record use annotation and they should only reflect on those fields. Code they cannot control (like core libs or other packages) they should not reflect on.
This is by design: For example I think it's perfectly fine for us to e.g. rename `_name` to `_enumName` here. It shouldn't impact or break anyone.
If you want link hooks to reflect on these fields, then we have to commit on making those fields always be there, never renamed, etc. In that case those two fields here should have a `@RecordUse` annotation and not a `@pragma('wasm:entry-point')`.
In any case, I don't see why handling of these fields should have any wasm specific treatments.
Also: Could you please add me as reviewer when changing dart2wasm related things (as I'm probably the most knowledgeable person for that) and alexmarkov when touching TFA (as he's the most knowledgeable person for that)?
I tried to reconstruct the index and name in another way in dart2wasm but wasn't able to find the right information in the ast at the point where the recordings are done.
(We explored an approach where we do all the recordings in the AST super early before any optimizations and TFA in the metadata repository and then trim it down later after TFA, but that also became very messy. So we went back to the approach where we record after TFA and reconstruct information that is lowered.)
Is there a way to get the name and index in dart2wasm without these entry-points?
Also: Could you please add me as reviewer when changing dart2wasm related things (as I'm probably the most knowledgeable person for that) and alexmarkov when touching TFA (as he's the most knowledgeable person for that)?
Yes, I'm chatting with Alex. We should probably have a chat as well about the high level approach things for dart2wasm where they don't align with the VM (which they don't in a bunch of cases). I'll shoot you a calendar invite.
Is there a way to get the name and index in dart2wasm without these entry-points?
The issue here has probably nothing to do with dart2wasm per se.
It's more by coincidence that it's not an issue for the VM: Even the smallest program (`main() {}`) compiled in product mode in VM AOT still has enums in them - namely instances of `const _OS(...)` and `const _Architecture(...)` and because there's multiple of those constants with different index/names we don't tree shake the fields.
Though one could argue a program that doesn't use FFI things should get ffi stuff tree shaken out. It doesn't happen atm because e.g. `_checkAbiSpecificIntegerMapping` has a `@pragma('vm:entry-point')` annotation and uses `Abi.current` internally which then depends on various enum constants.
If we made the ffi things not be retained if the program doesn't use ffi, then we may encounter the same issue that you encounter in dart2wasm also on the native AOT.
So as mentioned in previous comment: If at all this should be a `@RecordFieldUse` annotation (or something like) which applies to all backends equally.
Yes, I'm chatting with Alex. We should probably have a chat as well about the high level approach things for dart2wasm where they don't align with the VM (which they don't in a bunch of cases). I'll shoot you a calendar invite.
- Daco Harkes
Code-Owners
Code-Review
Commit-Message-Has-TEST
Core-Library-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |