[wasm] Load allocation space top and limit via root register [v8/v8 : main]
Clemens Backes (Gerrit)
Clemens Backes added 1 comment![]( "Open in Gerrit")
Matthias and Dominik, PTAL.
Matthias for wasm and compiler, Dominik for heap.
I think we can also do some follow-up cleanup to completely remove the external references for new/old top and limit. I'm working on that.
Related details
- Dominik Inführ
- Matthias Liedtke
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Darius Mercadier (Gerrit)
Darius Mercadier added 2 comments![]( "Open in Gerrit")
limit_address = __ ExternalConstant(
type == AllocationType::kYoung
? ExternalReference::new_space_allocation_limit_address(isolate_)
: ExternalReference::old_space_allocation_limit_address(
isolate_));same
top_address = __ ExternalConstant(
type == AllocationType::kYoung
? ExternalReference::new_space_allocation_top_address(isolate_)
: ExternalReference::old_space_allocation_top_address(isolate_));I would keep this for JS since it requires one less addition than the Wasm version.
Related details
- Clemens Backes
- Dominik Inführ
- Matthias Liedtke
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Clemens Backes (Gerrit)
Clemens Backes added 1 comment![]( "Open in Gerrit")
top_address = __ ExternalConstant(
type == AllocationType::kYoung
? ExternalReference::new_space_allocation_top_address(isolate_)
: ExternalReference::old_space_allocation_top_address(isolate_));I would keep this for JS since it requires one less addition than the Wasm version.
I was assuming that that would be folded away by a following step. In the end, it's "(kRootRegister + offset1) + offset2".
If not, we can also split the existing isolate fields in three, for the three pointers they contain. Then we can address each one directly.
Related details
- Darius Mercadier
- Dominik Inführ
- Matthias Liedtke
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Matthias Liedtke (Gerrit)
Matthias Liedtke added 1 comment![]( "Open in Gerrit")
static constexpr int kLimitOffset = 2 * kSystemPointerSize;Wouldn't it be much nicer to make them `static const` and define them out of line using `offsetof` removing the need for the `CheckOffsets` and the manual offset calculation?
We shouldn't need the `constexpr`-ness of these values.
https://godbolt.org/z/ofsf55T49
Related details
- Darius Mercadier
- Dominik Inführ
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Darius Mercadier (Gerrit)
Darius Mercadier added 2 comments![]( "Open in Gerrit")
limit_address = __ ExternalConstant(
type == AllocationType::kYoung
? ExternalReference::new_space_allocation_limit_address(isolate_)
: ExternalReference::old_space_allocation_limit_address(
isolate_));Darius Mercadiersame
As discussed above: all good.
top_address = __ ExternalConstant(
type == AllocationType::kYoung
? ExternalReference::new_space_allocation_top_address(isolate_)
: ExternalReference::old_space_allocation_top_address(isolate_));Clemens BackesI would keep this for JS since it requires one less addition than the Wasm version.
I was assuming that that would be folded away by a following step. In the end, it's "(kRootRegister + offset1) + offset2".
If not, we can also split the existing isolate fields in three, for the three pointers they contain. Then we can address each one directly.
Mmmh, indeed, the instruction selector will fold this if everything goes well. I'm wondering if we have instruction selector unit tests for this folding; if we don't, then we should (but no need to do it in this CL).
Ignore my initial comment then :)
Related details
- Clemens Backes
- Dominik Inführ
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dominik Inführ (Gerrit)
Dominik Inführ voted and added 2 comments![]( "Open in Gerrit")
Votes added by Dominik Inführ
| Code-Review | +1 |
2 comments
Cool, thanks a lot! LGTM
// Defined as a (static) class method, but outside the class, so we an verifyNit: Can
Related details
- Clemens Backes
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Clemens Backes (Gerrit)
Clemens Backes added 2 comments![]( "Open in Gerrit")
// Defined as a (static) class method, but outside the class, so we an verifyClemens BackesNit: Can
Obsolete (code is removed now).
static constexpr int kLimitOffset = 2 * kSystemPointerSize;Wouldn't it be much nicer to make them `static const` and define them out of line using `offsetof` removing the need for the `CheckOffsets` and the manual offset calculation?
We shouldn't need the `constexpr`-ness of these values.
https://godbolt.org/z/ofsf55T49
If we can make them `constexpr`, I would prefer that. It's also nice to write out the value explicitly, so it's easier to match that against generated code.
I moved the validation to the existing `Verify` method; turns out that you can call `offsetof` inside the class definition, just not outside of method bodies...
Is that acceptable to you?
Related details
- Matthias Liedtke
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Matthias Liedtke (Gerrit)
Matthias Liedtke voted and added 2 comments![]( "Open in Gerrit")
Votes added by Matthias Liedtke
| Code-Review | +1 |
2 comments
+1, I don't want to block this on a disagreement on an implementation detail vs. the clear and major benefit that the refactoring provides. 😊
static constexpr int kLimitOffset = 2 * kSystemPointerSize;Clemens BackesWouldn't it be much nicer to make them `static const` and define them out of line using `offsetof` removing the need for the `CheckOffsets` and the manual offset calculation?
We shouldn't need the `constexpr`-ness of these values.
https://godbolt.org/z/ofsf55T49
If we can make them `constexpr`, I would prefer that. It's also nice to write out the value explicitly, so it's easier to match that against generated code.
I moved the validation to the existing `Verify` method; turns out that you can call `offsetof` inside the class definition, just not outside of method bodies...
Is that acceptable to you?
Acceptable yes but I don't like it at all.
We shouldn't manually calculate offsets and make assumptions on C++ object layout which (afaik) is mostly defined by the ABI and not even defined on a C++ language level.
It also makes it annoying if we ever add members, reorder members etc. which would "just work" using `offsetof`.
The differences between `static const` and `static constexpr` are negligible in most cases (you can still use it as template arguments, size of a c-array, ...), so there isn't much benefit of `constexpr` here.)
In either case we should also add a `static_assert(std::is_standard_layout_v<LinearAllocationArea>);` to ensure that `offsetof` is well-defined.
Related details
- Clemens Backes
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Clemens Backes (Gerrit)
Clemens Backes added 1 comment![]( "Open in Gerrit")
static constexpr int kLimitOffset = 2 * kSystemPointerSize;Clemens BackesWouldn't it be much nicer to make them `static const` and define them out of line using `offsetof` removing the need for the `CheckOffsets` and the manual offset calculation?
We shouldn't need the `constexpr`-ness of these values.
https://godbolt.org/z/ofsf55T49
Matthias LiedtkeIf we can make them `constexpr`, I would prefer that. It's also nice to write out the value explicitly, so it's easier to match that against generated code.
I moved the validation to the existing `Verify` method; turns out that you can call `offsetof` inside the class definition, just not outside of method bodies...
Is that acceptable to you?
Acceptable yes but I don't like it at all.
We shouldn't manually calculate offsets and make assumptions on C++ object layout which (afaik) is mostly defined by the ABI and not even defined on a C++ language level.
It also makes it annoying if we ever add members, reorder members etc. which would "just work" using `offsetof`.The differences between `static const` and `static constexpr` are negligible in most cases (you can still use it as template arguments, size of a c-array, ...), so there isn't much benefit of `constexpr` here.)
In either case we should also add a `static_assert(std::is_standard_layout_v<LinearAllocationArea>);` to ensure that `offsetof` is well-defined.
I guess it's fine to disagree on this for now.
IMO the code in this CL is in line with existing V8 code. In particular, we already have the `kSize` constant and rely on statically knowing the layout of this class. The `IsolateData` class has two fields of type `LinearAllocationArea`, and then uses custom calculations (using `LinearAllocationArea::kSize`) to compute the offsets used in generated code for accessing fields inside these classes. So all of this only works if we fully control the memory layout. We explicitly do *not* want to automatically adapt to any changes which might be allowed by the C++ spec. Explicitly specifying offsets is easier to maintain and to debug in this case than deriving values automatically.
Happy to have more in-person discussion on this.
I added the static_assert out standard layout, which totally makes sense.
Related details
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Lippautz (Gerrit)
Michael Lippautz added 1 comment![]( "Open in Gerrit")
static constexpr int kStartOffset = 0;Drive-by: For the HeapObjectLayout stuff, I think we have established a convention where we
- Add static constexpr accessors that use offsetoff().
- Make those accessors private
- friend class the specific compiler parts that need the internals to make this explicit.
Related details
- Clemens Backes
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dominik Inführ (Gerrit)
Dominik Inführ voted and added 1 comment![]( "Open in Gerrit")
Votes added by Dominik Inführ
| Code-Review | +1 |
1 comment
static constexpr int kLimitOffset = 2 * kSystemPointerSize;Clemens BackesWouldn't it be much nicer to make them `static const` and define them out of line using `offsetof` removing the need for the `CheckOffsets` and the manual offset calculation?
We shouldn't need the `constexpr`-ness of these values.
https://godbolt.org/z/ofsf55T49
Matthias LiedtkeIf we can make them `constexpr`, I would prefer that. It's also nice to write out the value explicitly, so it's easier to match that against generated code.
I moved the validation to the existing `Verify` method; turns out that you can call `offsetof` inside the class definition, just not outside of method bodies...
Is that acceptable to you?
Clemens BackesAcceptable yes but I don't like it at all.
We shouldn't manually calculate offsets and make assumptions on C++ object layout which (afaik) is mostly defined by the ABI and not even defined on a C++ language level.
It also makes it annoying if we ever add members, reorder members etc. which would "just work" using `offsetof`.The differences between `static const` and `static constexpr` are negligible in most cases (you can still use it as template arguments, size of a c-array, ...), so there isn't much benefit of `constexpr` here.)
In either case we should also add a `static_assert(std::is_standard_layout_v<LinearAllocationArea>);` to ensure that `offsetof` is well-defined.
I guess it's fine to disagree on this for now.
IMO the code in this CL is in line with existing V8 code. In particular, we already have the `kSize` constant and rely on statically knowing the layout of this class. The `IsolateData` class has two fields of type `LinearAllocationArea`, and then uses custom calculations (using `LinearAllocationArea::kSize`) to compute the offsets used in generated code for accessing fields inside these classes. So all of this only works if we fully control the memory layout. We explicitly do *not* want to automatically adapt to any changes which might be allowed by the C++ spec. Explicitly specifying offsets is easier to maintain and to debug in this case than deriving values automatically.
Happy to have more in-person discussion on this.I added the static_assert out standard layout, which totally makes sense.
fwiw we already have this static_assert in IsolateData [0].
Clemens Backes (Gerrit)
Clemens Backes added 3 comments![]( "Open in Gerrit")
Michael, can you sign off src/execution and check if you like the latest changes?
Drive-by: For the HeapObjectLayout stuff, I think we have established a convention where we
- Add static constexpr accessors that use offsetoff().
- Make those accessors private
- friend class the specific compiler parts that need the internals to make this explicit.
Ah, right!
I like the solution with constexpr accessors. I don't really see the benefit of making the offsets private, by did that as well.
static constexpr int kLimitOffset = 2 * kSystemPointerSize;Clemens BackesWouldn't it be much nicer to make them `static const` and define them out of line using `offsetof` removing the need for the `CheckOffsets` and the manual offset calculation?
We shouldn't need the `constexpr`-ness of these values.
https://godbolt.org/z/ofsf55T49
Matthias LiedtkeIf we can make them `constexpr`, I would prefer that. It's also nice to write out the value explicitly, so it's easier to match that against generated code.
I moved the validation to the existing `Verify` method; turns out that you can call `offsetof` inside the class definition, just not outside of method bodies...
Is that acceptable to you?
Clemens BackesAcceptable yes but I don't like it at all.
We shouldn't manually calculate offsets and make assumptions on C++ object layout which (afaik) is mostly defined by the ABI and not even defined on a C++ language level.
It also makes it annoying if we ever add members, reorder members etc. which would "just work" using `offsetof`.The differences between `static const` and `static constexpr` are negligible in most cases (you can still use it as template arguments, size of a c-array, ...), so there isn't much benefit of `constexpr` here.)
In either case we should also add a `static_assert(std::is_standard_layout_v<LinearAllocationArea>);` to ensure that `offsetof` is well-defined.
Dominik InführI guess it's fine to disagree on this for now.
IMO the code in this CL is in line with existing V8 code. In particular, we already have the `kSize` constant and rely on statically knowing the layout of this class. The `IsolateData` class has two fields of type `LinearAllocationArea`, and then uses custom calculations (using `LinearAllocationArea::kSize`) to compute the offsets used in generated code for accessing fields inside these classes. So all of this only works if we fully control the memory layout. We explicitly do *not* want to automatically adapt to any changes which might be allowed by the C++ spec. Explicitly specifying offsets is easier to maintain and to debug in this case than deriving values automatically.
Happy to have more in-person discussion on this.I added the static_assert out standard layout, which totally makes sense.
fwiw we already have this static_assert in IsolateData [0].
Right. Still doesn't hurt to have it a second time where we use `offsetof` and again rely on this.
Related details
- Dominik Inführ
- Matthias Liedtke
- Michael Lippautz
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Clemens Backes (Gerrit)
Clemens Backes added 1 comment![]( "Open in Gerrit")
static constexpr int kStartOffset = 0;Clemens BackesDrive-by: For the HeapObjectLayout stuff, I think we have established a convention where we
- Add static constexpr accessors that use offsetoff().
- Make those accessors private
- friend class the specific compiler parts that need the internals to make this explicit.
Ah, right!
I like the solution with constexpr accessors. I don't really see the benefit of making the offsets private, by did that as well.
Actually, we can avoid most of the friendships (and more that would need to be added in https://crrev.com/c/6915236) by introducing new `IsolateFieldId`s for the top/limit of old and new space.
I did that in a separate CL, I think that makes things even nicer: https://crrev.com/c/6916875
Michael Lippautz (Gerrit)
Michael Lippautz voted Code-Review+1![]( "Open in Gerrit")
- Clemens Backes
- Dominik Inführ
- Matthias Liedtke
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Clemens Backes (Gerrit)
Clemens Backes voted Commit-Queue+2![]( "Open in Gerrit")
- Dominik Inführ
- Matthias Liedtke
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")
Change information
[wasm] Load allocation space top and limit via root register
We were currently storing pointers to the isolate fields on the
`WasmTrustedInstanceData`. This adds one unnecessary indirection, making
code larger and slower than needed.
All four fields are accessible via the roots register instead.
R=mlie...@chromium.org
CC=dinf...@chromium.org
- M src/compiler/memory-lowering.cc
- M src/compiler/turboshaft/memory-optimization-reducer.h
- M src/diagnostics/objects-printer.cc
- M src/execution/isolate-data.h
- M src/heap/linear-allocation-area.h
- M src/wasm/wasm-objects-inl.h
- M src/wasm/wasm-objects.cc
- M src/wasm/wasm-objects.h
Code-Review: +1 by Michael Lippautz
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |