[maglev] Port HeapNumber to the new VirtualObject model [v8/v8 : main]
Jakob Linke (Gerrit)
Jakob Linke added 1 comment![]( "Open in Gerrit")
// TODO(jgruber): Could we use the standard path below instead?I'm curious about this one. We already manually build inline allocs to handle mutable HN fields in the graph builder, but haven't checked yet how all the pieces fit together here.
Related details
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Victor Gomes (Gerrit)
Victor Gomes added 3 comments![]( "Open in Gerrit")
if (object->object_type() == vobj::ObjectType::kHeapNumber) {Can we check the map for HN and ConsString here instead of the type?
// TODO(jgruber): Could we use the standard path below instead?I'm curious about this one. We already manually build inline allocs to handle mutable HN fields in the graph builder, but haven't checked yet how all the pieces fit together here.
These are 2 different things. It depends if the object was elided or escaped. If we did emit the first one (and did not remove it, since we remove it later), the object is escaped, this will never be called.
Otherwise if the object was elided and we deopt, we need to re-materialize the object, this will ensure that.
kHeapNumber,I thought we would be able to get rid of these... :/
Related details
- Jakob Linke
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jakob Linke (Gerrit)
Jakob Linke voted and added 3 comments![]( "Open in Gerrit")
Votes added by Jakob Linke
| Auto-Submit | +1 |
3 comments
if (object->object_type() == vobj::ObjectType::kHeapNumber) {Can we check the map for HN and ConsString here instead of the type?
ConsString: no, the map may be non-constant. For HN, yes but IMO there is little benefit as long as we still take custom paths for materialization.
// TODO(jgruber): Could we use the standard path below instead?Victor GomesI'm curious about this one. We already manually build inline allocs to handle mutable HN fields in the graph builder, but haven't checked yet how all the pieces fit together here.
These are 2 different things. It depends if the object was elided or escaped. If we did emit the first one (and did not remove it, since we remove it later), the object is escaped, this will never be called.
Otherwise if the object was elided and we deopt, we need to re-materialize the object, this will ensure that.
Got it. The question remains if we can use the standard path, and it looks like "probably yes" if we tweak Float64Constant reification. But I'd like to look at that separately after FDA and HN have landed.
kHeapNumber,I thought we would be able to get rid of these... :/
Yeah it's unfortunate. With some effort we can unify a bit more, but as long as some types have special deopt handling we'll always have to distinguish between them.
On the upside, it's now clear where behavior diverges, instead of HN/CS/FDA being completely custom.
Related details
- Victor Gomes
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Victor Gomes (Gerrit)
Victor Gomes voted and added 2 comments![]( "Open in Gerrit")
Votes added by Victor Gomes
| Code-Review | +1 |
2 comments
if (object->object_type() == vobj::ObjectType::kHeapNumber) {Jakob LinkeCan we check the map for HN and ConsString here instead of the type?
ConsString: no, the map may be non-constant. For HN, yes but IMO there is little benefit as long as we still take custom paths for materialization.
Acknowledged
// TODO(jgruber): Could we use the standard path below instead?Victor GomesI'm curious about this one. We already manually build inline allocs to handle mutable HN fields in the graph builder, but haven't checked yet how all the pieces fit together here.
Jakob LinkeThese are 2 different things. It depends if the object was elided or escaped. If we did emit the first one (and did not remove it, since we remove it later), the object is escaped, this will never be called.
Otherwise if the object was elided and we deopt, we need to re-materialize the object, this will ensure that.
Got it. The question remains if we can use the standard path, and it looks like "probably yes" if we tweak Float64Constant reification. But I'd like to look at that separately after FDA and HN have landed.
- Jakob Linke
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jakob Linke (Gerrit)
Jakob Linke voted Commit-Queue+2![]( "Open in Gerrit")
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
V8 LUCI CQ (Gerrit)
V8 LUCI CQ submitted the change![]( "Open in Gerrit")
Unreviewed changes
3 is the latest approved patch-set.
No files were changed between the latest approved patch-set and the submitted one.
Change information
[maglev] Port HeapNumber to the new VirtualObject model
- M src/compiler/turboshaft/turbolev-graph-builder.cc
- M src/maglev/maglev-code-generator.cc
- M src/maglev/maglev-graph-builder.cc
- M src/maglev/maglev-graph-builder.h
- M src/maglev/maglev-ir.h
- M src/objects/heap-number.h
Code-Review: +1 by Victor Gomes
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Matthias Liedtke (Gerrit)
Matthias Liedtke has created a revert of this change![]( "Open in Gerrit")
Related details
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jakob Linke (Gerrit)
Jakob Linke added 1 comment![]( "Open in Gerrit")
PTAL. This is just a rebased version of https://chromium-review.googlesource.com/c/v8/v8/+/6975984.
Related details
- Victor Gomes
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jakob Linke (Gerrit)
Jakob Linke voted Auto-Submit+1![]( "Open in Gerrit")
| Auto-Submit | +1 |
Related details
- Victor Gomes
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Victor Gomes (Gerrit)
Victor Gomes voted and added 2 comments![]( "Open in Gerrit")
Votes added by Victor Gomes
| Code-Review | +1 |
2 comments
ForEachSlotIterationMode mode = ForEachSlotIterationMode::kDefault) {I wonder if all iterations should be kForDeopt iterations.
Related details
- Jakob Linke
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jakob Linke (Gerrit)
Jakob Linke voted and added 2 comments![]( "Open in Gerrit")
Votes added by Jakob Linke
| Commit-Queue | +2 |
2 comments
Thanks!
ForEachSlotIterationMode mode = ForEachSlotIterationMode::kDefault) {I wonder if all iterations should be kForDeopt iterations.
Usually it does make sense to visit all slots. Examples: clearing all slots in the ctor, and building stores for the InlinedAllocation.
Related details
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
V8 LUCI CQ (Gerrit)
V8 LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[maglev] Port HeapNumber to the new VirtualObject model
- M src/compiler/turboshaft/turbolev-graph-builder.cc
- M src/maglev/maglev-code-generator.cc
- M src/maglev/maglev-graph-builder.cc
- M src/maglev/maglev-graph-builder.h
- M src/maglev/maglev-ir.h
- M src/objects/heap-number.h
Code-Review: +1 by Victor Gomes
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |