[XL] Change in dart/sdk[main]: [vm,modular_aot] Boxing/unboxing of int and double values
Alexander Markov (Gerrit)
New activity on the change![]( "Open in Gerrit")
Related details
- Slava Egorov
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Slava Egorov (Gerrit)
Slava Egorov voted and added 3 comments![]( "Open in Gerrit")
Votes added by Slava Egorov
| Code-Review | +1 |
3 comments
int get hashCode => value.isNaN ? 31 : value.hashCode;Needs a comment to explain why `value.hashCode` is not good enough and why we need special case for NaN here. It's not obvious to me - I guess you want different NaNs to get the same hash code? `operator==` has no special provision and just uses `identical()` which would compare different NaNs as not-equal.
Phi() => _unboxedPhis[instr.id],I find it strange that this is not a property of the `Phi` means that after this pass we don't know anything about the type of the phi? Feels iffy.
Related details
- Alexander Markov
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alexander Markov (Gerrit)
Alexander Markov voted and added 4 comments![]( "Open in Gerrit")
Votes added by Alexander Markov
| Commit-Queue | +1 |
4 comments
Slava, thank you for taking a look!
Needs a comment to explain why `value.hashCode` is not good enough and why we need special case for NaN here. It's not obvious to me - I guess you want different NaNs to get the same hash code? `operator==` has no special provision and just uses `identical()` which would compare different NaNs as not-equal.
`value.hashCode` should be good enough, cleaned up this code.
_setNonConstant(instr);TODO, I guess?
My thinking is that constant propagation should be performed earlier in the pipeline, before box/unbox instructions are introduced (unboxing should happen very late in the pipeline). So I was borderline between adding `throw 'Unxpected'` and just making box/unbox instructions non-constant. Is there a good reason to perform constant propagation after unboxing too?
I find it strange that this is not a property of the `Phi` means that after this pass we don't know anything about the type of the phi? Feels iffy.
This property is preserved by keeping the whole `Unboxing` pass in the `BackEndState`, where it can be accessed by the back-end. Currently only regalloc uses it in order to pick FP register class for unboxed doubles.
I didn't add the unboxed property directly to `Phi` to avoid cluttering core back-end independent instruction with back-end specific details.
Related details
- Slava Egorov
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alexander Markov (Gerrit)
Alexander Markov voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
- Slava Egorov
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Commit Queue (Gerrit)
Commit Queue submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
2 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: pkg/cfg/lib/ir/constant_value.dart
Insertions: 1, Deletions: 1.
@@ -387,7 +387,7 @@
String toString() => 'UnboxedDoubleConstant($value)';
@override
- int get hashCode => value.isNaN ? 31 : value.hashCode;
+ int get hashCode => value.hashCode;
@override
bool operator ==(Object other) =>
```
Change information
[vm,modular_aot] Boxing/unboxing of int and double values
TEST=ci
Issue: https://github.com/dart-lang/sdk/issues/61635
Change-Id: I9fe47364bf772ccedf21f95aa5b16b92b0215b3e
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/486240
Commit-Queue: Alexander Markov <alexm...@google.com>
Reviewed-by: Slava Egorov <veg...@google.com>
- M pkg/cfg/lib/ir/constant_value.dart
- M pkg/cfg/lib/ir/flow_graph_checker.dart
- M pkg/cfg/lib/ir/instructions.dart
- M pkg/cfg/lib/ir/visitor.dart
- M pkg/cfg/lib/passes/constant_propagation.dart
- M pkg/cfg/lib/passes/simplification.dart
- M pkg/native_compiler/lib/back_end/arm64/assembler.dart
- M pkg/native_compiler/lib/back_end/arm64/code_generator.dart
- M pkg/native_compiler/lib/back_end/arm64/constraints.dart
- M pkg/native_compiler/lib/back_end/arm64/stub_code_generator.dart
- M pkg/native_compiler/lib/back_end/assembler.dart
- M pkg/native_compiler/lib/back_end/back_end_state.dart
- M pkg/native_compiler/lib/back_end/register_allocator.dart
- M pkg/native_compiler/lib/configuration.dart
- M pkg/native_compiler/lib/passes/lowering.dart
- A pkg/native_compiler/lib/passes/unboxing.dart
- M pkg/native_compiler/lib/snapshot/snapshot.dart
- M pkg/native_compiler/test/back_end/arm64/assembler_test.dart
- M pkg/native_compiler/test/back_end/arm64/disassembler.dart
- M pkg/native_compiler/test/ir_test.dart
- M pkg/native_compiler/testcases/lowering_test.dart.expect
- M pkg/native_compiler/testcases/reorder_blocks_test.dart.expect
- A pkg/native_compiler/testcases/unboxing_test.dart
- A pkg/native_compiler/testcases/unboxing_test.dart.expect
- M runtime/vm/module_snapshot.cc
Code-Review: +1 by Slava Egorov
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |