[turboshaft] Support trusted loads in late load elimination [v8/v8 : main]
Matthias Liedtke (Gerrit)
Matthias Liedtke voted and added 5 comments![]( "Open in Gerrit")
Votes added by Matthias Liedtke
| Commit-Queue | +1 |
5 comments
@dmerc...@chromium.org: PTAL. I had a successful test run of all CQ bots on patch set 7 prior to hiding the feature behind `--future` (which might limit some combinations of variants).
@nicoha...@chromium.org: Please component-review `src/base/macros.h` but you're welcome to take a look at the other parts of the CL as well. 😊
if (v8_flags.turboshaft_verify_load_elimination) {AFAICT this has zero coverage outside of fuzzers, right?
So we don't run this as part of any variant?
void EmitReportLoadEliminationError() {Mostly just moved (and slightly rewritten, so that the wasm part has an early return to reduce the amount of needed `#ifdef`s.)
EmitReportLoadEliminationError();How much do we think will it help to have different code locations for the different checks? After we had some OOM situations with this flag in Wasm in combination with another verification flag, would there bre any concerns about first joining all the error control flow and then emitting a single load? (I'd land that as a separate change.)
This would probably significantly reduce the graph size when the verification pass is enabled.
bool is_immutable,I focused on the case where this is `true`.
If it's true, this should mean we can either eliminate the whole load or there isn't much we can optimize.
I would hope that if this is `false`, the `ValueNumberingReducer` that runs in the same phase should still help us eliminating the shifts below if possible?
Related details
- Darius Mercadier
- Nico Hartmann
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| 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")
IF_SANDBOX(V, LoadTrustedPointerField)Quick follow-up on the name: I acknowledge that @sa...@chromium.org went for `Decode`, not `Load`. I think from a performance perspective, `Load` is a bit more honest? It also makes it clearer that this isn't deterministic based on its inputs?
I don't have a strong opinion though. 😊
Related details
- Darius Mercadier
- Nico Hartmann
- Samuel Groß
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Samuel Groß (Gerrit)
Samuel Groß added 1 comment![]( "Open in Gerrit")
IF_SANDBOX(V, LoadTrustedPointerField)Quick follow-up on the name: I acknowledge that @sa...@chromium.org went for `Decode`, not `Load`. I think from a performance perspective, `Load` is a bit more honest? It also makes it clearer that this isn't deterministic based on its inputs?
I don't have a strong opinion though. 😊
Yeah I think "load" is also fine. Elsewhere we also use "resolve" (e.g. https://source.chromium.org/chromium/chromium/src/+/main:v8/src/codegen/code-stub-assembler.h;l=1033;drc=54233b06577c8880f355dff17e33ef9c4075b082). I guess historically we've often used "decode" as a more opaque name if the operation behaved differently for sandbox vs no-sandbox configuration. But I guess here the new operation is only for the sandbox-enabled case.
Could you maybe still leave a TODO that it would be nice to have similar implementations for external pointers and trusted pointers? It's not super important, but would be nice to have, and is probably something that gemini could (soon) take care.
Related details
- Darius Mercadier
- Matthias Liedtke
- Nico Hartmann
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| 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")
IF_SANDBOX(V, LoadTrustedPointerField)Samuel GroßQuick follow-up on the name: I acknowledge that @sa...@chromium.org went for `Decode`, not `Load`. I think from a performance perspective, `Load` is a bit more honest? It also makes it clearer that this isn't deterministic based on its inputs?
I don't have a strong opinion though. 😊
Yeah I think "load" is also fine. Elsewhere we also use "resolve" (e.g. https://source.chromium.org/chromium/chromium/src/+/main:v8/src/codegen/code-stub-assembler.h;l=1033;drc=54233b06577c8880f355dff17e33ef9c4075b082). I guess historically we've often used "decode" as a more opaque name if the operation behaved differently for sandbox vs no-sandbox configuration. But I guess here the new operation is only for the sandbox-enabled case.
Could you maybe still leave a TODO that it would be nice to have similar implementations for external pointers and trusted pointers? It's not super important, but would be nice to have, and is probably something that gemini could (soon) take care.
Isn't there `DecodeExternalPointer` already for external pointers (which was the reason you wanted to add the same for trusted pointers in your CL?)
Trusted pointers are what this CL is about and protected pointers (am I confusing them again?) are just regular tagged loads just with a different base, aren't they? They don't need a separate instruction, they can use LoadOp` (and already do I think?) as other than the base they really don't have any magic tags, tables, ...
Related details
- Darius Mercadier
- Nico Hartmann
- Samuel Groß
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Darius Mercadier (Gerrit)
Darius Mercadier voted and added 6 comments![]( "Open in Gerrit")
Votes added by Darius Mercadier
| Code-Review | +1 |
6 comments
lgtm
if (v8_flags.turboshaft_verify_load_elimination) {AFAICT this has zero coverage outside of fuzzers, right?
So we don't run this as part of any variant?
Yea good point. Maybe we should group all of those debugging flags behind a single one that would be enabled in one of the variants. I'll do it when I have a moment.
void EmitReportLoadEliminationError() {Mostly just moved (and slightly rewritten, so that the wasm part has an early return to reduce the amount of needed `#ifdef`s.)
Acknowledged
EmitReportLoadEliminationError();How much do we think will it help to have different code locations for the different checks? After we had some OOM situations with this flag in Wasm in combination with another verification flag, would there bre any concerns about first joining all the error control flow and then emitting a single load? (I'd land that as a separate change.)
This would probably significantly reduce the graph size when the verification pass is enabled.
Yea perfectly fine, feel free to have a single place that emits the crashing code.
// We need to make sure that {load} and {replacement} have the same output
// representation. In particular, in unreachable code, it's possible thatYou're not doing this though. Either add a `DCHECK(RepIsCompatible)` or a branch of `RepIsCompatible` like ProcessLoad does (and/or update this comment)
bool is_immutable,I focused on the case where this is `true`.
If it's true, this should mean we can either eliminate the whole load or there isn't much we can optimize.
I would hope that if this is `false`, the `ValueNumberingReducer` that runs in the same phase should still help us eliminating the shifts below if possible?
Yea I was thinking about the same thing, and indeed I think that GVN should remove the redundant shifts.
Related details
- Nico Hartmann
- Samuel Groß
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Matthias Liedtke (Gerrit)
Matthias Liedtke added 2 comments![]( "Open in Gerrit")
if (v8_flags.turboshaft_verify_load_elimination) {Darius MercadierAFAICT this has zero coverage outside of fuzzers, right?
So we don't run this as part of any variant?
Yea good point. Maybe we should group all of those debugging flags behind a single one that would be enabled in one of the variants. I'll do it when I have a moment.
Sounds good, resolving here as this is independent of this change (and hopefully my change won't fail on all our fuzzers. If it does, I'll revert. 😊)
I checked locally and `--variants=turbolev_future --extra-flags=--turboshaft-verify-load-elimination` passes all tests.
// We need to make sure that {load} and {replacement} have the same output
// representation. In particular, in unreachable code, it's possible thatYou're not doing this though. Either add a `DCHECK(RepIsCompatible)` or a branch of `RepIsCompatible` like ProcessLoad does (and/or update this comment)
Yeah, this comment is a left-over, thanks for flagging, I adapted the comment and changed the `DCHECK` below.
Related details
- Nico Hartmann
- Samuel Groß
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Samuel Groß (Gerrit)
Samuel Groß added 1 comment![]( "Open in Gerrit")
IF_SANDBOX(V, LoadTrustedPointerField)Samuel GroßQuick follow-up on the name: I acknowledge that @sa...@chromium.org went for `Decode`, not `Load`. I think from a performance perspective, `Load` is a bit more honest? It also makes it clearer that this isn't deterministic based on its inputs?
I don't have a strong opinion though. 😊
Matthias LiedtkeYeah I think "load" is also fine. Elsewhere we also use "resolve" (e.g. https://source.chromium.org/chromium/chromium/src/+/main:v8/src/codegen/code-stub-assembler.h;l=1033;drc=54233b06577c8880f355dff17e33ef9c4075b082). I guess historically we've often used "decode" as a more opaque name if the operation behaved differently for sandbox vs no-sandbox configuration. But I guess here the new operation is only for the sandbox-enabled case.
Could you maybe still leave a TODO that it would be nice to have similar implementations for external pointers and trusted pointers? It's not super important, but would be nice to have, and is probably something that gemini could (soon) take care.
Isn't there `DecodeExternalPointer` already for external pointers (which was the reason you wanted to add the same for trusted pointers in your CL?)
Trusted pointers are what this CL is about and protected pointers (am I confusing them again?) are just regular tagged loads just with a different base, aren't they? They don't need a separate instruction, they can use LoadOp` (and already do I think?) as other than the base they really don't have any magic tags, tables, ...
Yeah I guess what I mean is that it would be nice to have
```
#define TURBOSHAFT_OTHER_OPERATION_LIST(V) \
...
V(XyzExternalPointer) \
V(XyzTrustedPointer)
```
Or
```
#define TURBOSHAFT_OTHER_OPERATION_LIST(V) \
...
IF_SANDBOX(V, XyzExternalPointerField) \
IF_SANDBOX(V, XyzTrustedPointerField)
```
Or something like that. Since these two things are basically the same except for which table they use, it would be nice to handle them in a similar way. Low priority, but maybe worth leaving a TODO.
Related details
- Matthias Liedtke
- Nico Hartmann
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Matthias Liedtke (Gerrit)
Matthias Liedtke voted and added 1 comment![]( "Open in Gerrit")
Votes added by Matthias Liedtke
| Auto-Submit | +1 |
1 comment
IF_SANDBOX(V, LoadTrustedPointerField)Samuel GroßQuick follow-up on the name: I acknowledge that @sa...@chromium.org went for `Decode`, not `Load`. I think from a performance perspective, `Load` is a bit more honest? It also makes it clearer that this isn't deterministic based on its inputs?
I don't have a strong opinion though. 😊
Matthias LiedtkeYeah I think "load" is also fine. Elsewhere we also use "resolve" (e.g. https://source.chromium.org/chromium/chromium/src/+/main:v8/src/codegen/code-stub-assembler.h;l=1033;drc=54233b06577c8880f355dff17e33ef9c4075b082). I guess historically we've often used "decode" as a more opaque name if the operation behaved differently for sandbox vs no-sandbox configuration. But I guess here the new operation is only for the sandbox-enabled case.
Could you maybe still leave a TODO that it would be nice to have similar implementations for external pointers and trusted pointers? It's not super important, but would be nice to have, and is probably something that gemini could (soon) take care.
Samuel GroßIsn't there `DecodeExternalPointer` already for external pointers (which was the reason you wanted to add the same for trusted pointers in your CL?)
Trusted pointers are what this CL is about and protected pointers (am I confusing them again?) are just regular tagged loads just with a different base, aren't they? They don't need a separate instruction, they can use LoadOp` (and already do I think?) as other than the base they really don't have any magic tags, tables, ...
Yeah I guess what I mean is that it would be nice to have
```
#define TURBOSHAFT_OTHER_OPERATION_LIST(V) \
...
V(XyzExternalPointer) \
V(XyzTrustedPointer)
```Or
```
#define TURBOSHAFT_OTHER_OPERATION_LIST(V) \
...
IF_SANDBOX(V, XyzExternalPointerField) \
IF_SANDBOX(V, XyzTrustedPointerField)
```Or something like that. Since these two things are basically the same except for which table they use, it would be nice to handle them in a similar way. Low priority, but maybe worth leaving a TODO.
Got it, then I also agree on the feasibility of telling Gemini to do it. 😊
I might just try it tomorrow if I don't get too much disrupted by other stuff.
Related details
- Nico Hartmann
- Samuel Groß
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| 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")
EmitReportLoadEliminationError();Darius MercadierHow much do we think will it help to have different code locations for the different checks? After we had some OOM situations with this flag in Wasm in combination with another verification flag, would there bre any concerns about first joining all the error control flow and then emitting a single load? (I'd land that as a separate change.)
This would probably significantly reduce the graph size when the verification pass is enabled.
Yea perfectly fine, feel free to have a single place that emits the crashing code.
- Samuel Groß
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nico Hartmann (Gerrit)
Nico Hartmann added 1 comment![]( "Open in Gerrit")
- Matthias Liedtke
- Samuel Groß
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nico Hartmann (Gerrit)
Nico Hartmann voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Matthias Liedtke
- Samuel Groß
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Matthias Liedtke (Gerrit)
Matthias Liedtke voted and added 1 comment![]( "Open in Gerrit")
Votes added by Matthias Liedtke
| Commit-Queue | +2 |
1 comment
- Samuel Groß
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
[turboshaft] Support trusted loads in late load elimination
This change adds a new LoadTrustedPointerField operation in Turboshaft
to support load elimination of trusted loads. These loads are very
relevant for Wasm, e.g. for inlined wrappers which need to follow a
long pointer chain with an intermediate trusted load to get from a
JSFunction to the actual Wasm code pointer on calling it.
For now, this new feature is staged behind --future (while the marking
of some of these loads inside the Wasm wrappers as immutable is
directly done without a feature flag.
- M src/base/macros.h
- M src/compiler/turboshaft/assembler.h
- M src/compiler/turboshaft/late-load-elimination-reducer.cc
- M src/compiler/turboshaft/late-load-elimination-reducer.h
- M src/compiler/turboshaft/load-store-simplification-reducer.h
- M src/compiler/turboshaft/operations.h
- M src/flags/flag-definitions.h
- M src/wasm/wrappers-inl.h
Code-Review: +1 by Nico Hartmann, +1 by Darius Mercadier
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |